Non-zero margin and non-static position for body breaks editor layout

TinyMCE-User commented 10 years ago

Description of problem:

Steps to reproduce:

Open http://fiddle.tinymce.com/imeaab
Open menu Table and insert 3x3 table.

Expected result:

The "Table" menu should open below the "Table" menu item. The submenu should open next to "Insert table" submenu.

Actual result:

The "Table" menu opens towards right from "Table" menu item. Offset seems to be around the amount of body element left margin. The submenu opens towards right from the "Insert table" submenu. Again, the extra offset seems to be amount of body element left margin (for total of twice of the body left margin compared to correct position).

For inline editor, the problem is even worse because the whole menu is also offset once.

TinyMCE-User commented 10 years ago

Another test case with inline editor: http://fiddle.tinymce.com/kmeaab

This test case seems to work correctly if viewport is narrow enough. With maximized browser window this very minimal test case shows major problems with the layout code.

TinyMCE-User commented 10 years ago

The "margin:auto" seems to be important trigger for the bug.

TinyMCE-User commented 10 years ago

Yet another test case without "margin:auto" and with "position:absolute": http://fiddle.tinymce.com/lmeaab

TinyMCE-User commented 10 years ago

Not even jQuery has solved that look at the yellow box here: http://api.jquery.com/offset/

Not sure if we will fix this one since: 1) It's the first time I've seen this reported. 2) If jQuery haven't solved it I doubt it's a common problem. 3) Can easily be avoided by modifying CSS and page layout add a wrapped div and align that.

TinyMCE-User commented 10 years ago

How about something like http://pastebin.com/HLQaqePm ? Seems to fix the issue for Google Chrome and adapting for other platforms should not be that much an effort.

TinyMCE-User commented 10 years ago

The problem is older browsers. IE 8 for example will return "auto" as it's computed style of marginLeft for the body.

TinyMCE-User commented 10 years ago

How about supporting at least modern browsers? Supporting modern IE, Gecko and Webkit should give pretty good coverage of real world browsers. I already provided support for Gecko...

TinyMCE-User commented 10 years ago

s/Gecko/Webkit/

TinyMCE-User commented 10 years ago

Nah. Fix your page instead. This is edge case implementation issue that I've never seen any other reports on nor has jQuery fixed it.

We are working on replacing parts of the DOMUtils core to use the jQuery compatible DomQuery and in the future the real jQuery for the jQuery build so we don't want to add a bunch of edge case logic in DOMUtils if we don't need to.

Super easy to fix in the implementation: http://fiddle.tinymce.com/smeaab

TinyMCE-User commented 10 years ago

Yeah, I'd love addding an another extra wrapper element around everything to workaround a single bug in TinyMCE code...

How about this patch: http://pastebin.com/M94BEgbh ?

The linked patch also contains explanation why the current implementation of DOMUtils::getPos() is broken in case the optimized code path is used. The correct behavior only needs one extra call to getBoundingClientRect() and two subtractions.

TinyMCE-User commented 10 years ago

s/addding/adding/ above and s/the skip the optimized/to skip the optimized/ in the patch.

TinyMCE-User commented 10 years ago

After applying that patch I got this result. http://screencast.com/t/eswBXHL3n

So it doesn't get the right position on Chrome/Gecko using a default margin in this case.

TinyMCE-User commented 10 years ago

I assume you mean the tooltip which is offset towards top and left by around 5 pixels?

Can you provide a link to test case that didn't work with the patch I provided? I retested the patch and it seems to work for me with both Google Chrome and Firefox using both inline:true and inline:false for TinyMCE.

Rendering engines tested: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

TinyMCE-User commented 10 years ago

Could you test the example cases in this bug with the following change in the code (getPos())

if (rootElm === doc.body && elm.getBoundingClientRect) {
if (false) {

which disables the optimized code path? At least that change fixes the bug I'm seeing. I'm trying to say that the optimization is broken as is. I understand that you would much prefer having the optimized code path fixed instead of removed.

TinyMCE-User commented 10 years ago

Okay, it seems that my patch breaks the case where has padding or has margin and both have position:static. The only way getPos() always works correctly if I disable the codepath using getBoundingClientRect()...

TinyMCE-User commented 10 years ago

Ok fixed this now. Apparently it was a more common issue than I thought. It takes the slow route when it detects the body not being static.

TinyMCE-User commented 10 years ago

OK. Thanks for the fix.

tinymce / tinymce

Non-zero margin and non-static position for body breaks editor layout #1756