1271714 - css nav displayed via hover doesn't stay open when moving the mouse to the opened subnav

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jean Claveau]

Attachment: nav-bug-firefox.7z

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160425115046

Steps to reproduce:

Place the mouse above an item of the nav. The subnav is displayed. 

Then move the mouse to the opened subnav. 




Actual results:

The subnav disapears when the mouse enters.


Expected results:

It must remain clickable.

The example is in the attachment as I can't provide a public access.

I saw this bug in the developer edition for a while now and thought it was due to e10s but it looks it isn't as it appears since today with the release.

Comment 1

[Loic]

It would be helpful to have a testcase with a simplified CSS sheet.

Comment 2

[Jean Claveau]

Attachment: Bug example with lighter css inline

Comment 3

[Loic]

Thanks for the reduced testcase, I can reproduce it. I'm going to find a regression range.

Comment 4

[Loic]

Attachment: bug-firefox.html (reduced testcase)

Comment 5

[Loic]

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=35b211eaad1fa828064514c547057e4400e24459

Maybe:
Mats Palmgren — Bug 1151243 part 4 - Some code cleanup in nsHTMLReflowState::CalculateHypotheticalPosition, and make a few methods 'const' (idempotent patch). r=dholbert

Comment 6

[Mats Palmgren (inactive)]

I backed out bug 1151243 locally but that didn't help.

Perhaps it's bug 1147673?

Comment 7

[Loic]

I ran Mozreg against:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2ec54b
38a33da939a8255612999d9e867eb11664&tochange=bb5becd378f40a9be14e4e635d7034f2835f
c7b5

You're right, it's bug 1147673.

Comment 8

[Markus Stange [:mstange]]

Attachment: testcase

Thank you for the testcase. I've reduced it some more.
It looks like hit testing doesn't find the fixed element at all.

Comment 9

[Markus Stange [:mstange]]

This sounds similar to bug 1265237, but isn't fixed by it.

Comment 10

[Timothy Nikkel (:tnikkel)]

Edited display list

  SubDocument f=0x12217ca08(Viewport(-1)) bounds(0,4740,76800,42360)
    BackgroundColor f=0x12217d558(HTMLScroll(html)(-1)) bounds(0,4740,76800,42360)
    CanvasBackgroundColor f=0x12217d1f0(Canvas(html)(-1)) bounds(0,4740,76800,42360)
    BackgroundColor f=0x1230e5268(Block(html)(-1)) bounds(0,4740,76800,960)
    BackgroundColor f=0x1230e5930(Block(body)(1)) bounds(480,5220,75840,0)
    WrapList f=0x124f18260(HTMLScroll(deckiv)(0) class:positionedClip) z=1 bounds(480,5220,12120,0)
      BackgroundColor f=0x124f18260(HTMLScroll(div)(0) class:positionedClip) z=1 bounds(480,5220,75840,0)
      BackgroundColor f=0x124f187b0(Block(div)(0) class:positionedClip) bounds(480,5220,75840,0)
-->   WrapList f=0x124f18d18(Block(div)(1) class:relative) z=1 bounds(480,5220,12120,3120) scrollClip(<480,5220,75840,0>, <0,4740,76800,42360>)
        BackgroundColor f=0x124f18d18(Block(div)(1) class:relative) z=1 bounds(480,5220,75840,0)
        WrapList f=0x124f19178(Block(div)(1) class:fixed) bounds(480,5220,12120,3120) scrollClip(<0,4740,76800,42360>)
          BackgroundColor f=0x124f19178(Block(div)(1) class:fixed) bounds(480,5220,12120,3120) scrollClip(<0,4740,76800,42360>)
          Border f=0x124f19178(Block(div)(1) class:fixed) bounds(480,5220,12120,3120)scrollClip(<0,4740,76800,42360>)

I think the item with the arrow beside it is the problem. It has non-empty bounds, but has an empty scroll clip, so it contributes nothing to the parent wraplist's bounds, and so we never enter the parent wrap list (hit testing empty bounds fails).

The wraplist for the fixed content of course doesn't have the empty scroll clip, but yet it is in a display list with the empty scroll clip (that doesn't apply to it).

Comment 11

[Markus Stange [:mstange]]

Thanks! For container items that get created in nsIFrame::BuildDisplayListForStackingContext, we avoid this problem by calling clipState.ExitStackingContextContents which picks the "ancestor scroll clip" for the stacking context's contents. It looks like we're creating the WrapList in a place where we don't do this.

Comment 12

[Marcia Knous [:marcia]]

Let's track this regression for 48/49.

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 14

[Markus Stange [:mstange]]

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

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

Thanks! We need a test though. I'll try to convert the testcase into one.

Comment 15

[Markus Stange [:mstange]]

Attachment: test

Comment 17

[Timothy Nikkel (:tnikkel)]

Thanks for the test. I was planning on writing one, just wanted to get the patch up asap.

Comment 18

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58ceb872225

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/557c5dbd25a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f66a5b9da35f

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d01ac7bd7b34b1270020ded21d67fdc71a34fd7
Back out bug 1271714 (changeset f66a5b9da35f and changeset 557c5dbd25a1) for causing unexpected assertions in Android debug crashtest and reftest.

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6d01ac7bd7b3

See the commit message of the backout changeset for details on the failures.

Comment 22

[Liz Henry (:lizzard)]

Wontfix for 46; we only have a couple of weeks left until the 47 release.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This one might be too late for 47. Markus, Tim, is there a patch that needs to land here after the backout mentioned in comment 21?

Comment 24

[Timothy Nikkel (:tnikkel)]

Still working on fixing the problem that caused the backout.

Comment 25

[Timothy Nikkel (:tnikkel)]

We're building the display list for the scrollbars of a scrollable div. The DisplayListClipState::AutoClipMultiple in nsFrame::BuildDisplayListForChild init's mStackingContextAncestorSC to null. Scrollbars are DISPLAY_CHILD_FORCE_STACKING_CONTEXT so we create a wrapper item at the end of nsFrame::BuildDisplayListForChild using this null clip. Before my patch the clip would be the innermost scroll clip (which contains the clip from a parent scrollable div). Let's call this innermost scroll clip at this point in time C.

This changes the bounds of the nsDisplayWrapList item because nsDisplayWrapList::UpdateBounds calls GetScrollClippedBoundsUpTo(aBuilder, mScrollClip) to compute it's bounds. All of the child items of the wraplist have scrollclip == C. So when mScrollClip of the wraplist is C this is just the union of the bounds of the child items. When mScrollClip of the wraplist is nullptr this is the bounds of C.

The wraplist item gets flattened in ProcessDisplayItems, but the nsDisplayOwnLayer that is it's parent has already inherited the bounds of the wrap list item.

Comment 26

[Timothy Nikkel (:tnikkel)]

Seems like init'ing mStackingContextAncestorSC to GetCurrentInnermostScrollClip (so it's valid and innermost before EnterStackingContext is called) would be the right thing to do. I'll see if try agrees.

Comment 27

[Timothy Nikkel (:tnikkel)]

That causes some more of the same assertions to fire on try, this time on linux. I'll look into those now.

Comment 28

[Timothy Nikkel (:tnikkel)]

Bug 1285409 fixes the asserts, yay!

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: addasserts

One test still asserts and I don't know of any way to fix it. The asserts don't point out a real rendering problem, just a way in which our bookkeeping can't keep up with reality.

Comment 30

[Markus Stange [:mstange]]

Comment on attachment 8769302 [details] [diff] [review]
addasserts

That's fine. I haven't thought through this specific case completely, but since I'm touching all that code again at the moment, maybe things will look different when I'm done and we can revisit this test.

Comment 31

[Timothy Nikkel (:tnikkel)]

Attachment: stackcontextclipinit

This is the fix from comment 36 that I forgot to get review on.

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8769997 [details] [diff] [review]
stackcontextclipinit

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

I need to play with this patch a little before I know whether it's a good idea.

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #32)
> I need to play with this patch a little before I know whether it's a good
> idea.

If it helps I think my patch in this bug introduces the only user that would use the value that is initialized in the AutoSaveRestore constructor. I think all the other users would call EnterStackingContext first, which initializes this scroll clip variable.

Comment 34

[Markus Stange [:mstange]]

Comment on attachment 8769997 [details] [diff] [review]
stackcontextclipinit

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

Ooh yes, that does help.

Comment 35

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #31)
> Created attachment 8769997 [details] [diff] [review]
> stackcontextclipinit
> 
> This is the fix from comment 36 that I forgot to get review on.

This should be comment 26.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e8a6311551
Annotate expected asserts in layout/printing/crashtests/576878.xhtml. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d582c239872
When creating the wrap list item that contains the display list for a frame make sure to use a scroll clip that includes content in the display list. r=mstange

Comment 37

[Carsten Book [:Tomcat]]

sorry had to back this out for issues like https://treeherder.mozilla.org/logviewer.html#?job_id=31703706&repo=mozilla-inbound

Comment 38

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7138905f2c
Backed out changeset e0e8a6311551

Comment 39

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b19a16a27943
Backed out changeset 0d582c239872 for test failures

Comment 40

[Timothy Nikkel (:tnikkel)]

Apparently this doesn't cause those asserts to fire anymore.

Comment 41

[Timothy Nikkel (:tnikkel)]

I can still reproduce the asserts locally when applying on tip, so the asserts weren't fixed, just for some reason that I don't understand they don't show up when run on the automation machines.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3792b13e443
When creating the wrap list item that contains the display list for a frame make sure to use a scroll clip that includes content in the display list. r=mstange

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3acbc708b6
Annotate expected asserts in layout/printing/crashtests/576878.xhtml. r=mstange

Comment 44

[Timothy Nikkel (:tnikkel)]

The asserts are now happening on inbound. I have no idea why. I pushed the assert patch again.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd53db031e
Limit asserts annotations for crashtest to e10s only, cause that's where they happen.

Comment 46

[Timothy Nikkel (:tnikkel)]

Ah, the asserts only happen on e10s. That's why I was confused.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3792b13e443
https://hg.mozilla.org/mozilla-central/rev/8e3acbc708b6
https://hg.mozilla.org/mozilla-central/rev/77bd53db031e

Comment 48

[Liz Henry (:lizzard)]

Timothy is this something you may want to uplift? Probably too late for beta (heading into beta 9 now) but we could possibly take it on aurora for 49 if you think it's worth the risk.

Comment 49

[Timothy Nikkel (:tnikkel)]

Yeah, I think we should uplift to aurora. This has shipped on release already, but uplifting to aurora means we can get the fix out one release quicker.

Comment 50

[Timothy Nikkel (:tnikkel)]

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

Approval Request Comment
[Feature/regressing bug #]: bug 1147673
[User impact if declined]: some css fixed position menus might not display
[Describe test coverage new/current, TreeHerder]: there is a test, I just haven't gotten to landing it yet
[Risks and why]: should be pretty safe
[String/UUID change made/needed]: none

Comment 51

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #49)
> Yeah, I think we should uplift to aurora. This has shipped on release
> already, but uplifting to aurora means we can get the fix out one release
> quicker.

By "This has shipped on release already" I meant that this regression has shipped on release.

Comment 52

[Jet Villegas (inactive)]

Marking 'wontfix' in 48 but we still want uplift approval for 49 per comment 50.

Comment 53

[Gerry Chang [:gchang]]

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

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

This patch fixes a regression. Take it in aurora.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/db9f8c2dcb83
https://hg.mozilla.org/releases/mozilla-aurora/rev/29dda3d9dfcb

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b52c454783c
Test.

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b52c454783c