Closed Bug 1575138 Opened 6 years ago Closed 6 years ago

When opening a sub-menu in the Bookmarks or History menus, first item isn't focused

Categories

(Toolkit :: UI Widgets, defect, P2)

Product:
Toolkit
Component:
UI Widgets
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- verified
firefox71 --- verified

People

(Reporter: Jamie, Assigned: emilio)

References

(Regression)

Details

(Keywords: access, regression)

Attachments

(4 files)

Bug 1575138 - match poupshowing event handling in menupopup CE with XBL menupoup
47 bytes, text/x-phabricator-request
Details | Review
Bug 1575138 - remove setting of non existing event listeners in places-menupopup widget
47 bytes, text/x-phabricator-request
Details | Review
Bug 1575138 - Don't bother scheduling a reconstruct on <slot>s that have no fallback.
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review
Bug 1575138 - Do not schedule reconstruction for <slot> if there's no fallback.
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review

STR:

  1. Open a tab and close it.
  2. Press alt+s, then r, then enter to open the Recently Closed Tabs sub-menu of the History menu.
  3. Press enter.
    • Expected: The menu should be dismissed and the tab you closed in step 1 should reopen.
    • Actual: Nothing happens. The menu isn't dismissed.

You have to press down arrow to focus the first item. Even more confusingly, when the sub-menu opens, accessibility does report the first menu item as focused. So, a user ends up pressing enter or down arrow and gets nothing and wonders what's going on.

I can't reproduce on osx. Click History menu, press r to select Recently Closed Tabs submenu, hit Enter to open it, hit Enter - History menu is closed, tab is open.

Also noticed that History menu may have Restore session menuitem which is also associated with 'r' accesskey and takes precedence over Recently Closed Tabs, but I suppose it's your case, right?

Are there any ideas, what can be special on Windows platform, what leads to the bug?

setting p2 since this regression affects keyboard navigation user experience badly, and thus it should be fixed in the same release as the guilty bug. I wonder, if anyone with Windows machine can shed light on what happens here.

Priority: -- → P2

(In reply to alexander :surkov (:asurkov) from comment #1)

I can't reproduce on osx. Click History menu, press r to select Recently Closed Tabs submenu, hit Enter to open it, hit Enter - History menu is closed, tab is open.

We don't use places-popup for this menu on mac:
https://searchfox.org/mozilla-central/rev/03853a6e87c4a9405fce1de49e5d03b9e7a7a274/browser/base/content/browser-menubar.inc#268

Also noticed that History menu may have Restore session menuitem which is also associated with 'r' accesskey and takes precedence over Recently Closed Tabs, but I suppose it's your case, right?

That's not the problem here, no; that item is invisible for me at present.

If I change things so that #historyUndoPopup gets populated in #goPopup onpopupshowing (instead of #historyUndoPopup onpopupshowing), this fixes the issue. So, it seems repopulating the popup in onpopupshowing causes breakage. Here's a hacky patch to demonstrate the hack:

diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc
index f025314a25475..072e86074477c 100644
--- a/browser/base/content/browser-menubar.inc
+++ b/browser/base/content/browser-menubar.inc
@@ -240,7 +240,8 @@
                          oncommand="this.parentNode._placesView._onCommand(event);"
                          onclick="checkForMiddleClick(this, event);"
                          onpopupshowing="if (!this.parentNode._placesView)
-                                           new HistoryMenu(event);"
+                                           new HistoryMenu(event);
+                                           if (event.target == this) document.getElementById('history-menu')._placesView.populateUndoSubmenu();"
                          tooltip="bhTooltip"
                          popupsinherittooltip="true">
                 <menuitem id="menu_showAllHistory"
@@ -267,7 +268,7 @@
                              placespopup="true"
                              is="places-popup"
 #endif
-                             onpopupshowing="document.getElementById('history-menu')._placesView.populateUndoSubmenu();"/>
+                             />
                 </menu>
                 <menu id="historyUndoWindowMenu"
                       disabled="true" data-l10n-id="menu-history-undo-window-menu">

I don't think this is a feasible solution, but it might give you some starting points.

[Tracking Requested - why for this release]: Accessibility regression in a core browser feature.

status-firefox70: --- → affected
tracking-firefox70: --- → ?

Hello! Here is the regression range made on Windows 10x64:

Last good revision: 8dd0375cfaf81d140c19c98f8a9dda28b9a201c1
First bad revision: 047e16b38566f319ee1a9d566dc9b7e7c82bdd7d
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8dd0375cfaf81d140c19c98f8a9dda28b9a201c1&tochange=047e16b38566f319ee1a9d566dc9b7e7c82bdd7d

I used STR from comment 0 but after pressing ALT+S I pressed "R" key twice to select "Recently Closed Tabs". There were cases where after opening the "Recently Closed Tabs" the closed tab from the top was focused but pressing "Enter" key does nothing. Pressing UP arrow key after opening "Recently Closed Tabs" creates another focus item which works as default for "Enter" key and going on the first focused item removes one of the focuses. Attached a screen recording with the behavior.

Has Regression Range: --- → yes
Keywords: regressionwindow-wanted

While trying to get a working build on vmware, getting stuck with code reading. So, a menuitem gets focused, when a popup is shown (after popupshowing event is fired) [1], so when menuitems are repopulated on popupshowing, then nothing should prevent them from being focused on popupshown. I'm also puzzled how the CE conversion could affect on behavior: menupopup shadow DOM stuff happen at connectionCallback, which is sync, so it shouldn't interfere with focusing on popupshow.

Brian, is it about right? Can you see anything suspicious with places-menupopup CE vs XBL?

[1] https://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#865-866

Pls see question in comment #6

Flags: needinfo?(bgrinstead)

(In reply to alexander :surkov (:asurkov) from comment #6)

While trying to get a working build on vmware, getting stuck with code reading. So, a menuitem gets focused, when a popup is shown (after popupshowing event is fired) [1], so when menuitems are repopulated on popupshowing, then nothing should prevent them from being focused on popupshown. I'm also puzzled how the CE conversion could affect on behavior: menupopup shadow DOM stuff happen at connectionCallback, which is sync, so it shouldn't interfere with focusing on popupshow.

Brian, is it about right? Can you see anything suspicious with places-menupopup CE vs XBL?

[1] https://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#865-866

I actually think Jaime's discovery in Comment 3 hints at the solution. I notice the XBL menupopup has [phase=target] on the popupshowing handler, but we don't early return in the CE event listener when e.target != this:

I would guess if you add that early return to the CE event listener it should fix the problem.

Flags: needinfo?(bgrinstead)

(In reply to Brian Grinstead [:bgrins] from comment #8)

I actually think Jaime's discovery in Comment 3 hints at the solution. I notice the XBL menupopup has [phase=target] on the popupshowing handler, but we don't early return in the CE event listener when e.target != this:

this indeed improves things, however it doesn't fix the issue: pressing enter it opens a menupoup and select a menuitem, however subsequent enter doesn't active the selected menuitem. A little debugging shows that mCurrentMenu is null [1], when enter key is handled by popup manager [2]. It seems it fails to set a menuitem, when the menu is open by a shortcut 'r' key. Emilio, curious, if you have ideas on what may happen here.

[1] https://searchfox.org/mozilla-central/source/layout/xul/nsMenuPopupFrame.cpp#1971
[2] https://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#2216

Flags: needinfo?(emilio)
Pushed by asurkov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ccafed667d71 match poupshowing event handling in menupopup CE with XBL menupoup r=bgrins

Is it possible that the fix from Comment 3 would still apply even with this initial fix? That is, the [onpopupshowing] listener in the markup is now seeing an event bubble out from a child, but before when it was in XBL it wouldn't fire? So we'd need to somehow prevent the events from child nodes from propagating.

I'd need to check when mCurrentMenu is cleared out, or how is it selected otherwise. Maybe here?

But not sure off-hand, would need some proper debugging and I'll be on PTO until Thursday. Please let me know by ni?ing back if you want me to dig into that :)

Flags: needinfo?(emilio)

(In reply to Brian Grinstead [:bgrins] from comment #12)

Is it possible that the fix from Comment 3 would still apply even with this initial fix? That is, the [onpopupshowing] listener in the markup is now seeing an event bubble out from a child, but before when it was in XBL it wouldn't fire? So we'd need to somehow prevent the events from child nodes from propagating.

I'd be surprised if XBL affected on event propagation, how could it be, I don't see any related stopPropogation calls in code? Which markup onpopupshowing event you to refer to? goPopup which creates HistoryMenu object? I think I miss you idea.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

I'd need to check when mCurrentMenu is cleared out, or how is it selected otherwise. Maybe here?

you're right, the stack is:

xul.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent, nsCSSFrameConstructor::InsertionKind aInsertionKind) Line 8733	C++
xul.dll!mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList & aChangeList) Line 1568	C++
xul.dll!mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags aFlags) Line 3114	C++
xul.dll!mozilla::RestyleManager::ProcessPendingRestyles() Line 3196	C++
xul.dll!mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 4127	C++

I'm curious how to find out what those pending restyles are, and why menu current item doesn't survive frames recreation, I suppose that restyling shouldn't drop menu state.

But not sure off-hand, would need some proper debugging and I'll be on PTO until Thursday. Please let me know by ni?ing back if you want me to dig into that :)

it'd very helpful indeed

Flags: needinfo?(emilio)
Pushed by asurkov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f1bcdfebf34c remove setting of non existing event listeners in places-menupopup widget r=bgrins

(In reply to alexander :surkov (:asurkov) from comment #15)

(In reply to Brian Grinstead [:bgrins] from comment #12)

Is it possible that the fix from Comment 3 would still apply even with this initial fix? That is, the [onpopupshowing] listener in the markup is now seeing an event bubble out from a child, but before when it was in XBL it wouldn't fire? So we'd need to somehow prevent the events from child nodes from propagating.

I'd be surprised if XBL affected on event propagation, how could it be, I don't see any related stopPropogation calls in code? Which markup onpopupshowing event you to refer to? goPopup which creates HistoryMenu object? I think I miss you idea.

I was thinking that this event would be stopped by XBL automatically from elements inside <content> (which is how this used to be set up). But I did a quick test and it seems they do fire on the outer node. Anyway, I guess I'm curious then if the fix from Comment 3 fully fixes the problem, or if it only got us into the current state (after https://bugzilla.mozilla.org/show_bug.cgi?id=1575138#c11 landed).

Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED

(In reply to Brian Grinstead [:bgrins] from comment #19)

I was thinking that this event would be stopped by XBL automatically from elements inside <content> (which is how this used to be set up). But I did a quick test and it seems they do fire on the outer node. Anyway, I guess I'm curious then if the fix from Comment 3 fully fixes the problem, or if it only got us into the current state (after https://bugzilla.mozilla.org/show_bug.cgi?id=1575138#c11 landed).

It fixes the problem. Do you think this is acceptable solution? I suppose the RecentlyClosed popup content cannot change when History popup is open. So the only downside of the approach I can see is we populate content of the popup which may never be used, but I don't think it will have any user negative impact.

Besides that, it's weird I don't understand why we destroy a menu frame in this case. It might be some layout issue here, better to fix now so it won't suddenly popup in another place.

(In reply to alexander :surkov (:asurkov) from comment #20)

(In reply to Brian Grinstead [:bgrins] from comment #19)

I was thinking that this event would be stopped by XBL automatically from elements inside <content> (which is how this used to be set up). But I did a quick test and it seems they do fire on the outer node. Anyway, I guess I'm curious then if the fix from Comment 3 fully fixes the problem, or if it only got us into the current state (after https://bugzilla.mozilla.org/show_bug.cgi?id=1575138#c11 landed).

It fixes the problem. Do you think this is acceptable solution? I suppose the RecentlyClosed popup content cannot change when History popup is open. So the only downside of the approach I can see is we populate content of the popup which may never be used, but I don't think it will have any user negative impact.

Besides that, it's weird I don't understand why we destroy a menu frame in this case. It might be some layout issue here, better to fix now so it won't suddenly popup in another place.

It's good to know that it fixes the problem - worst case, we could land and uplift that to fix the remaining issue. But until we know exactly why it's happening this could surface in other places, so fixing the root cause is still needed.

This one is a bit of an odd one, lot's of fun stuff going on...

So basically what's going on is that we remove all children from the popup here:

This makes us schedule a reconstruct of the slot, in case it has fallback content:

Then we insert frames for the items. They get inserted right away, because we don't support lazy frame construction for XUL:

If this was normal HTML content, the insertion would've been lazy, and no reconstruct would've happened. Since we construct them, we now destroy them because of the reconstruct that was scheduled on the slot. That clears the active state because XUL and $reasons:

So, ways to fix this:

  • In this case there's no fallback possible, we could just avoid reconstructing the <slot>. Easy, but not very future-proof.
  • (better): Start supporting lazy frame constructions for (some) XUL elements.
Flags: needinfo?(emilio)

So basically what's going on is that we remove all children from the popup here:

https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/browser/base/content/browser-places.js#687

This makes us schedule a reconstruct of the slot, in case it has fallback
content:

https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/dom/base/ShadowRoot.cpp#616

Then we insert frames for the items. They get inserted right away, because we
don't support lazy frame construction for XUL:

https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/layout/base/nsCSSFrameConstructor.cpp#6507

If this was normal HTML content, the insertion would've been lazy, and no
reconstruct would've happened.

The right fix is to support lazy frame construction for XUL. Now that there's
very little XBL it should be possible. This fixes it but it's a kind-of stop-gap
solution.

This is a try run with lazy frame construction unconditionally enabled for XUL:

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

These days I think it should be much better than before. Is this something that the de-XBL people could help get over the finish line?

Flags: needinfo?(bgrinstead)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

This is a try run with lazy frame construction unconditionally enabled for XUL:

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

These days I think it should be much better than before. Is this something that the de-XBL people could help get over the finish line?

Do you anticipate breakage to be mostly XBL-related? If so, then blocking this on de-xbl and waiting to get down to 0 bindings seems reasonable (since it's on the order of weeks at this point). If there's more general breakage we could probably take a look at it anyway, after bindings are gone.

Flags: needinfo?(bgrinstead) → needinfo?(emilio)

(In reply to Brian Grinstead [:bgrins] from comment #26)

Do you anticipate breakage to be mostly XBL-related? If so, then blocking this on de-xbl and waiting to get down to 0 bindings seems reasonable (since it's on the order of weeks at this point). If there's more general breakage we could probably take a look at it anyway, after bindings are gone.

Yes.

This is needed, in theory (and practice too hopefully), because of people relying on XBL bindings being loaded as soon as elements were inserted into the document. So less bindings == less breakage, I'd hope ;)

A good first step may be to change that IsXULElement check for a whitelist of stuff that doesn't cause problems or some sort...

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #27)

(In reply to Brian Grinstead [:bgrins] from comment #26)

Do you anticipate breakage to be mostly XBL-related? If so, then blocking this on de-xbl and waiting to get down to 0 bindings seems reasonable (since it's on the order of weeks at this point). If there's more general breakage we could probably take a look at it anyway, after bindings are gone.

Yes.

This is needed, in theory (and practice too hopefully), because of people relying on XBL bindings being loaded as soon as elements were inserted into the document. So less bindings == less breakage, I'd hope ;)

Oh yeah, if that's the case then I definitely expect less breakage. The only tag names we still use xbl on are seen in https://searchfox.org/mozilla-central/search?q=-moz-binding&path=.css:

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79b08a0a1c3f Don't bother scheduling a reconstruct on <slot>s that have no fallback. r=smaug

Just realized that we probably want this too.

Attachment #9094808 - Attachment description: Bug 1575138 - Do the same but for insertion. → Bug 1575138 - Do not schedule reconstruction for <slot> if there's no fallback.

This should be fixed once those land.

Assignee: surkov.alexander → emilio
Keywords: leave-open
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a0d009f763b Do not schedule reconstruction for <slot> if there's no fallback. r=smaug

https://hg.mozilla.org/mozilla-central/rev/9a0d009f763b

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

This is a try run with lazy frame construction unconditionally enabled for XUL:

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

These days I think it should be much better than before. Is this something that the de-XBL people could help get over the finish line?

Would you mind filing a bug blocking Bug 1566221 to make this change? I'd be happy to help get it over the finish line once we don't have any more bindings.

Flags: needinfo?(emilio)
See Also: → 1584935
Flags: needinfo?(emilio)

Comment on attachment 9094808 [details]
Bug 1575138 - Do not schedule reconstruction for <slot> if there's no fallback.

Beta/Release Uplift Approval Request

  • User impact if declined: See comment 0.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Trivial optimization that papers over XUL bug.
  • String changes made/needed: none
Attachment #9094808 - Flags: approval-mozilla-beta?
Attachment #9094678 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Build ID 20191002033852
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0

Verified as fixed on the latest Nightly build on Windows 10.

status-firefox71: fixed → verified

Comment on attachment 9094808 [details]
Bug 1575138 - Do not schedule reconstruction for <slot> if there's no fallback.

Fix for focus, verified in nightly, let's uplift for beta 12.

Attachment #9094808 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9094678 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1578111

Build ID 20191007220302
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Verified as fixed on the latest Beta build (70.0b12) on Windows 10.

Status: RESOLVED → VERIFIED
status-firefox70: fixed → verified
tracking-firefox70: + → ---
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: