Closed Bug 1530594 Opened 9 months ago Closed 8 months ago

<menulist> in Site Identity panel does not respond to arrow keys when collapsed

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox66 + fixed
firefox67 --- verified

People

(Reporter: Jamie, Assigned: enndeakin)

References

Details

(Keywords: access)

Attachments

(2 files)

Spun off bug 1522092 comment 33.

Preparation:
Disable PanelMultiView's keydown handler so that it doesn't override the arrow keys. (Otherwise, you'll hit bug 1522092.) You can do this by commenting out addEventListener for keydown here:
https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1117
Then build Firefox!

STR:

  1. Visit this URL: http://www.popuptest.com/popuptest1.html
  2. Open the Site Identity panel.
  3. Tab to the Open Pop-up Windows permission menulist. Ensure "Block" is chosen.
  4. Press up arrow.
    • Expected: Allow should be selected in the menulist.
    • Actual: Nothing happens.

The menulist won't change the selection unless I open it and close it (e.g. with f4 then escape). After I do this once while the panel is open, the arrow keys continue to work while the menulist is closed, even if I tab away and back again. I tracked this down to setting .activeChild on the menulist failing:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/menulist.js#50

this.activeChild = this.mSelectedInternal;

Even though mSelectedInternal is correct, getting activeChild immediately afterwards returns null. I haven't worked out why yet. I guess nsMenuPopupFrame::ChangeMenuItem doesn't like this for some reason, but I can't work out why. It behaves just fine for the menulists in the Options dialog, for example.

Neil noted (in bug 1522092 comment 33):

Considering that when I try it the menulist size jumps about as I change the current selection, it looks like it might not be picking up the sizetopopup properly. Could be a regression from bug 1519917.

Impact: This is part of the cause for these permission selectors (and thus part of our new block autoplay feature) being keyboard inaccessible.

It's a bit late in our 66 beta cycle but I'd still take a patch for this. I'd like us to launch the feature with keyboard navigation working; it may be a blocker for the feature itself.

Neil, can you help find an owner for this issue?

Flags: needinfo?(enndeakin)

This is caused by bug 1411290 which for some reason assigned sizetopopup="none" to this menulist. This will cause it to not generate any frames for any of the menulist children until it is opened. I'm not sure why that bug changes the menulist to sizetopopup="none", but I have a patch which generates the frames when one assigns the active child to make keyboard navigation work properly.

Flags: needinfo?(enndeakin)

Sounds promising!

Assignee: nobody → enndeakin
Status: NEW → ASSIGNED

+Johann to review

Flags: needinfo?(jhofmann)

Nope, that was bug 1522092, this one's for tnikkel :)

Flags: needinfo?(jhofmann)
Flags: needinfo?(tnikkel)

Clearing needinfo assuming it was about the review.

Flags: needinfo?(tnikkel)
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e63fa41257b
generate frames before setting the active child in a menulist, so that menulists with sizetopopup='none' will still allow keyboard navigation when the menulist has not yet been opened, r=tnikkel

Backed out changeset 2e63fa41257b (Bug 1530594) for mochitest failure use-after-poison /builds/worker/workspace/build/src/layout/xul/nsMenuPopupFrame.h:280:42 in PopupType

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=2e63fa41257b8950f5726fd88936ba3f5e2d8441

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231415314&repo=mozilla-inbound&lineNumber=5684

Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=812a2e26c05ea230f0104d2a30a19ff5d8ba7529

[task 2019-03-02T03:59:30.898Z] 03:59:30 INFO - TEST-START | toolkit/content/tests/chrome/test_popuphidden.xul
[task 2019-03-02T03:59:31.126Z] 03:59:31 INFO - GECKO(6189) | =================================================================
[task 2019-03-02T03:59:31.128Z] 03:59:31 ERROR - GECKO(6189) | ==6189==ERROR: AddressSanitizer: use-after-poison on address 0x625000f2878c at pc 0x7f0244cde3a4 bp 0x7ffd41d00170 sp 0x7ffd41d00168
[task 2019-03-02T03:59:31.130Z] 03:59:31 INFO - GECKO(6189) | READ of size 4 at 0x625000f2878c thread T0
[task 2019-03-02T03:59:31.810Z] 03:59:31 INFO - GECKO(6189) | #0 0x7f0244cde3a3 in PopupType /builds/worker/workspace/build/src/layout/xul/nsMenuPopupFrame.h:280:42
[task 2019-03-02T03:59:31.810Z] 03:59:31 INFO - GECKO(6189) | #1 0x7f0244cde3a3 in nsXULPopupManager::FirePopupShowingEvent(nsIContent*, bool, bool, mozilla::dom::Event*) /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp:1333
[task 2019-03-02T03:59:31.811Z] 03:59:31 INFO - GECKO(6189) | #2 0x7f0244cde7a6 in nsXULPopupManager::ShowPopup(nsIContent*, nsIContent*, nsTSubstring<char16_t> const&, int, int, bool, bool, bool, mozilla::dom::Event*) /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp:711:3
[task 2019-03-02T03:59:31.827Z] 03:59:31 INFO - GECKO(6189) | #3 0x7f0243a63a09 in mozilla::dom::XULPopupElement::OpenPopup(mozilla::dom::Element*, mozilla::dom::StringOrOpenPopupOptions const&, int, int, bool, bool, mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/xul/XULPopupElement.cpp:65:9
[task 2019-03-02T03:59:31.888Z] 03:59:31 INFO - GECKO(6189) | #4 0x7f0240f434b1 in mozilla::dom::XULPopupElement_Binding::openPopup(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XULPopupElement*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XULPopupElementBinding.cpp:709:9
[task 2019-03-02T03:59:31.904Z] 03:59:31 INFO - GECKO(6189) | #5 0x7f024192deae in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3144:13
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #6 0x7f0247e5ea87 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:440:13
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #7 0x7f0247e5ea87 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:532
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #8 0x7f0247e4682f in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:591:10
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #9 0x7f0247e4682f in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3055
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #10 0x7f0247e294a8 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:420:10
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #11 0x7f0247e5f3f6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:560:13
[task 2019-03-02T03:59:31.926Z] 03:59:31 INFO - GECKO(6189) | #12 0x7f0247e61042 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:603:8
[task 2019-03-02T03:59:31.962Z] 03:59:31 INFO - GECKO(6189) | #13 0x7f0248a38d26 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2623:10
[task 2019-03-02T03:59:32.059Z] 03:59:32 INFO - GECKO(6189) | #14 0x7f02410edf6f in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:266:37
[task 2019-03-02T03:59:32.082Z] 03:59:32 INFO - GECKO(6189) | #15 0x7f02420a6a4f in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
[task 2019-03-02T03:59:32.082Z] 03:59:32 INFO - GECKO(6189) | #16 0x7f02420a4351 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/workspace/build/src/dom/events/JSEventHandler.cpp:205:12
[task 2019-03-02T03:59:32.085Z] 03:59:32 INFO - GECKO(6189) | #17 0x7f024206a356 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1042:51
[task 2019-03-02T03:59:32.086Z] 03:59:32 INFO - GECKO(6189) | #18 0x7f024206c222 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1237:17
[task 2019-03-02T03:59:32.086Z] 03:59:32 INFO - GECKO(6189) | #19 0x7f0242052cac in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:350:5
[task 2019-03-02T03:59:32.087Z] 03:59:32 INFO - GECKO(6189) | #20 0x7f0242052cac in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:351
[task 2019-03-02T03:59:32.088Z] 03:59:32 INFO - GECKO(6189) | #21 0x7f0242051495 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:553:16
[task 2019-03-02T03:59:32.089Z] 03:59:32 INFO - GECKO(6189) | #22 0x7f0242056dac in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1048:11
[task 2019-03-02T03:59:32.090Z] 03:59:32 INFO - GECKO(6189) | #23 0x7f0244ce36a6 in nsXULPopupManager::HidePopupCallback(nsIContent
, nsMenuPopupFrame*, nsIContent*, nsIContent*, nsPopupType, bool) /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp:1053:3
[task 2019-03-02T03:59:32.090Z] 03:59:32 INFO - GECKO(6189) | #24 0x7f0244ce22c2 in nsXULPopupManager::FirePopupHidingEvent(nsIContent*, nsIContent*, nsIContent*, nsPresContext*, nsPopupType, bool, bool) /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp:1466:7
[task 2019-03-02T03:59:32.094Z] 03:59:32 INFO - GECKO(6189) | #25 0x7f0244cd71d9 in nsXULPopupManager::HidePopup(nsIContent*, bool, bool, bool, bool, nsIContent*) /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp:968:7
[task 2019-03-02T03:59:32.096Z] 03:59:32 INFO - GECKO(6189) | #26 0x7f0243a6400a in mozilla::dom::XULPopupElement::HidePopup(bool) /builds/worker/workspace/build/src/dom/xul/XULPopupElement.cpp:96:9
[task 2019-03-02T03:59:32.097Z] 03:59:32 INFO - GECKO(6189) | #27 0x7f0240f46cc8 in mozilla::dom::XULPopupElement_Binding::hidePopup(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XULPopupElement*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XULPopupElementBinding.cpp:940:9
[task 2019-03-02T03:59:32.103Z] 03:59:32 INFO - GECKO(6189) | #28 0x7f024192deae in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3144:13
[task 2019-03-02T03:59:32.104Z] 03:59:32 INFO - GECKO(6189) | #29 0x7f0247e5ea87 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:440:13

Flags: needinfo?(enndeakin)

Neil, it sounds like we need this along with bug 1522092 to be uplifted to 66 if we're going to have an accessible launch of the block auto-play feature. Are you going to be able to get this landed quickly so we've got a shot at uplift?

Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7395290c18
generate frames before setting the active child in a menulist, so that menulists with sizetopopup='none' will still allow keyboard navigation when the menulist has not yet been opened, updated to get popup type earlier to avoid possible crash, r=tnikkel
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Thanks, Neil. I've verified that this works as expected in the Site Identity panel in Nightly 20190305214137. Could you please request uplift ASAP? My approval comment for the other patch is bug 1522092 comment 49 if that's helpful.

The patch changes code that was changed around in https://phabricator.services.mozilla.com/D19489 which doesn't exist in beta, so I need to create a new patch.

Flags: needinfo?(enndeakin)

...false from IsLeafDynamic anyway, the only effect is that this one menulist is now not a leaf, allowing its content to be generated upfront. This particular menulist only has two items in it anyway so there shouldn't be a performance issue, r?tnikkel

Neil, please request uplift for beta.

Flags: needinfo?(enndeakin)

The patch is waiting for review.

Flags: needinfo?(enndeakin)

I'll check back over the weekend. Maybe we can find another reviewer if tnikkel is out.

Maria, can you or someone on your team verify the fix? Thanks!

Flags: needinfo?(maria.berlinger)

Comment on attachment 9049209 [details]
Bug 1530594, treat menulists as always being leaf frames. This is a simpler patch that should work for beta. As the menulist that is the subject of this bug is the only one that has sizetopopup='none', and the remaining menulists would return...

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: Alllow/Block dropdown in identity panel won't change options until menulist is opened, affecting accessibility.
  • 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: A similar but different fix is in Nightly. The automated test from that patch could be moved over. Manual testing done.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed: None
Attachment #9049209 - Flags: approval-mozilla-beta?

I can add the automated test from the main patch to the beta patch by tomorrow morning.

The patch ( https://phabricator.services.mozilla.com/D21515 ) now includes the automated test, but I haven't run it except locally. Note that this issue does not affect Mac as you cannot adjust dropdown values without opening them on Mac.

Comment on attachment 9049209 [details]
Bug 1530594, treat menulists as always being leaf frames. This is a simpler patch that should work for beta. As the menulist that is the subject of this bug is the only one that has sizetopopup='none', and the remaining menulists would return...

Thanks! Let's uplift for the RC.

Attachment #9049209 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I've managed to reproduce this issue on an affected Firefox 66.0b13 build using Windows 10x64 by following the STR from comment 0.
This is verified fixed using Firefox 67.0a1(20190310214003) on the following OSes: Windows 10 x64, Ubuntu 18.04 x64 and Windows 7 x64.

Status: RESOLVED → VERIFIED
Flags: needinfo?(maria.berlinger)
You need to log in before you can comment on or make changes to this bug.