Opened 10 years ago

Closed 10 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:

  1. Remove the <!doctype> line from sample.html.
  2. Open replacebyclass.html in IE6, 7 or 8.
  3. All IE versions -> The position of the context menu is wrong.
  4. IE6 and 7 -> The internal layout of the context menu is also wrong.

Attachments (4)

3545.patch (5.5 KB) - added by Tobiasz Cudnik 10 years ago.
Tested on FF3, IE6, IE7, IE8.
test_element_getDocumentPosition.patch (2.4 KB) - added by Martin Kou 10 years ago.
3545_2.patch (5.4 KB) - added by Tobiasz Cudnik 10 years ago.
3545_3.patch (5.0 KB) - added by Tobiasz Cudnik 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: newassigned

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3545.patch added

Tested on FF3, IE6, IE7, IE8.

comment:2 Changed 10 years ago by Tobiasz Cudnik

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 10 years ago by Martin Kou

Keywords: 3.0RC added

Marking it as 3.0RC since this issue is important.

Changed 10 years ago by Martin Kou

comment:4 Changed 10 years ago by Martin Kou

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:

  1. Click Size combo box to open.
  2. Click Format combo box to open.

Changed 10 years ago by Tobiasz Cudnik

Attachment: 3545_2.patch added

comment:5 Changed 10 years ago by Tobiasz Cudnik

Keywords: Review? added; Review- removed

comment:6 Changed 10 years ago by Martin Kou

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...

  1. _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.
  2. _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.
  3. _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( ... );
    
  4. _source/core/dom/element.js, lines 1124 - 1140. Since getBoundingClientRect() is supported by Opera, this can be safely deleted.

comment:7 Changed 10 years ago by Tobiasz Cudnik

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 10 years ago by Tobiasz Cudnik

Attachment: 3545_3.patch added

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:9 Changed 10 years ago by Tobiasz Cudnik

Resolution: fixed
Status: assignedclosed

Fixed with [3626].

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy