Opened 16 years ago
Closed 15 years ago
#3545 closed Bug (fixed)
Context menus have wrong layout in IE quirks mode.
Reported by: | Martin Kou | Owned by: | Tobiasz Cudnik |
---|---|---|---|
Priority: | Must have (possibly next milestone) | Milestone: | CKEditor 3.0 |
Component: | General | Version: | SVN (CKEditor) - OLD |
Keywords: | Confirmed IE Review+ 3.0RC | Cc: |
Description
To reproduce:
- Remove the <!doctype> line from sample.html.
- Open replacebyclass.html in IE6, 7 or 8.
- All IE versions -> The position of the context menu is wrong.
- IE6 and 7 -> The internal layout of the context menu is also wrong.
Attachments (4)
Change History (13)
comment:1 Changed 16 years ago by
Owner: | set to Tobiasz Cudnik |
---|---|
Status: | new → assigned |
Changed 16 years ago by
Attachment: | 3545.patch added |
---|
comment:2 Changed 16 years ago by
Keywords: | Review? added |
---|
I had to introduce changes to CKEDITOR.dom.element.getDocumentPosition which take use of getBoundingClientRect function. Being in IE since IE5, it also ships with FF3. Although everything seems to work fine with it, extensive tests of all other position-affected parts of code base are required.
Reason for this was bug in offsetTop of legend element created on L198 of wysiwygarea plugin. It was possible to workaround it, but general solution was a better choice and can also speed up position-related operations (since there's no loop).
There is also still problem with height measuring, probably related with #3426. Because of this, scrollbar was appearing. This is (scrollbar, not height) also fixed by this patch.
comment:3 Changed 16 years ago by
Keywords: | 3.0RC added |
---|
Marking it as 3.0RC since this issue is important.
Changed 16 years ago by
Attachment: | test_element_getDocumentPosition.patch added |
---|
comment:4 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
The getDocumentPosition() logic seems ok (see attached test case, recycled from #3034). However, the patch is causing JavaScript error in IE in the following case:
- Click Size combo box to open.
- Click Format combo box to open.
Changed 16 years ago by
Attachment: | 3545_2.patch added |
---|
comment:5 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
comment:6 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
The position in IE quirks mode is correct, but under IE standards mode is wrong - as well as some other browsers like Firefox 3.
The problem here is that after the patch, we're now mixing up the logic that's designed for offsetTop/offsetLeft and the position returned by getBoundingClientRect(). It seems to me that getDocumentPosition() should be split into two cases for browsers that support getBoundingClientRect() and for those who don't. The split can be done in the same way getComputedStyle() is split into two cases.
And with the split up, there will be quite a few areas where the code has to be changed as well...
- _source/core/dom/element.js, lines 1161 - 1174. Since all IE versions will use getBoundingClientRect(), it no longer makes sense to check for IE here.
- _source/core/dom/element.js, lines 1176 - 1183. This applies to FF2 only, FF3 using getBoundingClientRect() no longer has this error. So this section of code can be safely moved to the "without getBoundingClientRect()" case.
- _source/plugins/menu/plugin.js, line 161. It's wasteful to have two element.getDocument() calls - the getDocument() call isn't compressed by CKReleaser. Using getElementsByTag() to retrieve the HTML tag is also wasteful. I'd suggest something like this:
var body = element.getDocument().getBody(); body.setStyle( ... ); body.getParent().setStyle( ... );
- _source/core/dom/element.js, lines 1124 - 1140. Since getBoundingClientRect() is supported by Opera, this can be safely deleted.
comment:7 Changed 15 years ago by
Keywords: | Review? added; Review- removed |
---|
I've applied you suggestions, but done the split using standard conditional statement to omit code repetition which is still used in both cases.
Tested on all mentioned browsers. Seems to work always.
I would prefer to move layout issues to #3563 which completely changes menu.css. I will merge actual changes with that ticket.
Changed 15 years ago by
Attachment: | 3545_3.patch added |
---|
comment:8 Changed 15 years ago by
Keywords: | Review+ added; Review? removed |
---|
Tested on FF3, IE6, IE7, IE8.