1322257 - [non-e10s] The <select> box dropdown list is rendered detached from the <select> element contained in iframe

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Ajay Pal Singh Atwal]

Attachment: How Select Dropdown Renders (incorrectly)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

Create two iframes in a page, one a menu other work area
add some CSS to remove borders, spaces, full height/ width for iframes
displaye a page containing a select drop down in each iframe (same page in both frames for simplicity)



Actual results:

On clicking the select element, the select box drop down is rendered detached from the select element. If I choose inspect element by right clicking on the select element, the rendering becomes as expected i.e drop down attached to select element


Expected results:

Select drop down should have appeared adjacent/ attached to the select element

Comment 1

[Ajay Pal Singh Atwal]

Attachment: Expected Rendering of Select Drop down

Comment 2

[Ajay Pal Singh Atwal]

Attachment: The HTML defining iframes for test case

Comment 3

[Ajay Pal Singh Atwal]

Attachment: select.html

this html is rendered in an iframe through the index.html

Comment 4

[Loic]

Attachment: index.html

Comment 5

[Loic]

It's rendered fine with e10s enabled.

Comment 6

[YF (Yang)]

Attachment: Screenshot in Fx53 e10s on

I see a strange results. In my main profile, it works fine with e10s off, but it does not work if saved as a local files or use firefox safe mode. In a new profile (e10s off), it resembles the result of saving to local files. The above tests are use comment 4 with 51.0b6 (64-bit) on Win10.

In addition, in 53.0a1 (2016-12-07) (32-bit) with e10s on, it is normal location but slightly larger, see screenshot.

Comment 7

[Ajay Pal Singh Atwal]

Please note that if I right click and choose inspect element the select rendering becomes as expected in non e10s, not only locally but online as well and remains OK till next refresh of page. 

If I remove two lines from css in index.html
/* comment out these two fixes the issue */
   margin: 0px;
   padding: 0px;
/* */

This bug is not triggered

e10s rendering is OK

Comment 8

[Ajay Pal Singh Atwal]

Attachment: This renders <select> correctly after removing a few CSS properties

This file contains same iframes but two CSS properties removed i.e.
   margin: 0px;
   padding: 0px;
It renders <select> correctly in non e10s

Comment 9

[Timothy Nikkel (:tnikkel)]

Caused by bug 594333. Confirmed by reverting it locally.

Comment 10

[Timothy Nikkel (:tnikkel)]

nsTableCellFrame::Reflow computes the position of its child, then calls nsContainerFrame::ReflowChild with that position. nsContainerFrame::ReflowChild moves the child and positions the view of that child if it has a view (it doesn't have a view in this case, so no views are moved, including views in the frame subtree rooted at the child). Then it calls nsContainerFrame::FinishReflowChild with the same position. FinishReflowChild checks to see if it moved the frame, and if so it positions the view for the child (PositionFrameView) and any descendants (PositionChildViews). But ReflowChild already moved the child to this position, so FinishReflowChild sees no change in position and no views are moved. It seems like there is a bug in ReflowChild, it should be calling PositionChildViews as well as PositionFrameView.

Some users of ReflowChild/FinishReflowChild seem to also do view positioning themselves. Some don't.

nsContainerFrame::ReflowChild is ancient code, so changing it is scary. One possible bad scenario that is possible:
1) child is moved from it's eventually final position to a non-final position
2) ReflowChild is called for the child when it is in this non-final position (views are moved)
3) child is moved back to final position without using/calling FinishReflowChild)
4) the code doesn't do any positioning of views for this final move, so the descendant views are in the wrong place

A less scary patch would be to check the intial and final position in nsTableCellFrame::Reflow, and position views if they are different.


The history of this code goes back to 1999:

http://searchfox.org/mozilla-central/commit/597b5f236b0455bdda4ff91da00fe94a9958e63a

And that commit in nsTableCellFrame specifically at the ReflowChild callsite

http://searchfox.org/mozilla-central/diff/597b5f236b0455bdda4ff91da00fe94a9958e63a/layout/tables/nsTableCellFrame.cpp#668

there is an interesting comment that was added with that commit above the ReflowChild call:

+   // Assume the inner child will stay positioned exactly where it is. Later in
+   // VerticallyAlignChild() we'll move it if it turns out to be wrong. This
+   // avoids excessive movement and is more stable

and the position passed to ReflowChild is the existing position of the child (unless this is the first reflow, in which case it computes it). So the code was expecting the child to not move, and hence to not need to move views.

dbaron's giant reflow refactor from bug 300030 rewrote this code:

http://searchfox.org/mozilla-central/diff/31f1898810f733c40bfe3fd4b25295885feb2e39/layout/tables/nsTableCellFrame.cpp#812

The comment was removed and the position was always computed.

Comment 11

[Timothy Nikkel (:tnikkel)]

Both suggested fixes from comment 10 pass try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c93cc615b1f0528428b300b97c04ee6d3d18f351
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67f5f68911dc80c6a990dabb8a78926068f2af36

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Sorry to dump this on you dholbert.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8818800 [details] [diff] [review]
patch

Review of attachment 8818800 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks -- this seems fine to me.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31212fd22bb
Always position child views in nsContainerFrame::ReflowChild. r=dholbert

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d31212fd22bb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 17

[Timothy Nikkel (:tnikkel)]

This is an old bug (6 years old) and the fix is a little scary so I'd prefer not to uplift if that's okay. Less and less users will see this as e10s rolls out to more people anyway.

Comment 18

[Paul Silaghi, QA [:pauly]]

Reproduced on Fx 52.
Verified fixed Fx 53b1 Win 10, Ubuntu 14.04, OS X 10.11.