[e10s] optgroup in <select> should not be selectable

VERIFIED FIXED in Firefox 41

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: alice0775, Assigned: enndeakin)

Tracking

(Blocks 2 bugs)

34 Branch
mozilla41
x86_64
Windows 7
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify ?

Firefox Tracking Flags

(e10sm8+, firefox41 fixed)

Details

(Whiteboard: [testday-20150821])

Attachments

(4 attachments, 4 obsolete attachments)

Posted image screenshot
optgroup in <select> should not be selectable.

STR
mouse up/down
Posted file select.html
Blocks: fxe10s
Assignee: nobody → mrbkap
Depends on: old-e10s-m2
Blocks: old-e10s-m2
No longer depends on: old-e10s-m2
Posted patch menucaption (obsolete) — Splinter Review
This patch fixes three bugs:

1. Adds a caption attribute for menuitems so that the captions can be distinguished and not selected. They also have to be disabled, otherwise I'd need to change all the native theme code to handle this.
2. Doesn't allow the checkmark on the captions.
3. Switches to check for the optgroup tag directly rather than the child count so that empty groups are handled properly.
Assignee: mrbkap → enndeakin
Status: NEW → ASSIGNED
Move old M2's low-priority bugs to M6 milestone.
Neil, is this patch ready for review?
Flags: needinfo?(enndeakin)
I'll update it and try it out.
Flags: needinfo?(enndeakin)
Iteration: --- → 39.3 - 30 Mar
Points: --- → 3
Flags: firefox-backlog+
Flags: qe-verify?
Posted patch Updated patch (obsolete) — Splinter Review
This patch fixes some issues with the selected index when reopening and adds a test
Attachment #8483537 - Attachment is obsolete: true
Posted patch Fix leakSplinter Review
Running the test causes a failure due to SelectParentHelper not being cleared.
Attachment #8580017 - Flags: review?(felipc)
Blocks: 1116038
Attachment #8580017 - Flags: review?(felipc) → review+
Just to make sure I understand, what is leaked is the SelectParentHelper obj itself, right? And the prob could also be fixed by just importing it to a temporary obj instead of `this`?
Iteration: 39.3 - 30 Mar → 39.2 - 23 Mar
Comment on attachment 8580015 [details] [diff] [review]
Updated patch

This adds a caption="true" for menuitems that are captions.
Attachment #8580015 - Flags: review?(neil)
Attachment #8580015 - Flags: review?(felipc)
Comment on attachment 8580015 [details] [diff] [review]
Updated patch

This is used for <optgroup> headers. I'm not sure what should be used for accessibility. The caption is skipped in normal navigation, but the user may want to read it.
Attachment #8580015 - Flags: review?(surkov.alexander)
Comment on attachment 8580015 [details] [diff] [review]
Updated patch

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

::: accessible/xul/XULMenuAccessible.cpp
@@ +116,5 @@
>  XULMenuitemAccessible::NativeInteractiveState() const
>  {
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::caption,
> +                            nsGkAtoms::_true, eCaseMatters)) {
> +    return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE;

it should be rather opposite, neither of states is expected, that's how we handle the HTML optgroup. Also it should expose roles::GROUPING (XULMenuitemAccessible::NativeRole())

could you please add a11y for tests it,
1) states into states/test_controls.xul
2) hierarchy into tree/test_combobox.xul

I don't see any examples, so if captioned menuitem doesn't contain its items as children then their group position should be fixed (you don't have to fix it in this bug).

r=me with comment addressed, thanks for fixing a11y!
Attachment #8580015 - Flags: review?(surkov.alexander) → review+
you know if it doesn't contain items as children then let's not change role for now because grouping containing no items is weird (let's have a follow up on it)
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Attachment #8580015 - Flags: review?(felipc) → review+
(In reply to Neil Deakin from comment #9)
> This adds a caption="true" for menuitems that are captions.

Is there a good reason to use a menuitem for an optgroup? I'm thinking that the menu code would automatically skip the element if it was, say, a menucaption. (Of course you would want to bind the XBL and add it to the themes appropriately.)
(In reply to neil@parkwaycc.co.uk from comment #13)
> (In reply to Neil Deakin from comment #9)
> > This adds a caption="true" for menuitems that are captions.
> 
> Is there a good reason to use a menuitem for an optgroup? I'm thinking that
> the menu code would automatically skip the element if it was, say, a
> menucaption. (Of course you would want to bind the XBL and add it to the
> themes appropriately.)

I don't recall why now but I can try to create a separate element instead.
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Posted patch Add a menucaption element (obsolete) — Splinter Review
Attachment #8580015 - Attachment is obsolete: true
Attachment #8580015 - Flags: review?(neil)
Attachment #8587420 - Flags: review?(neil)
I'll do the accessibility part separately.
Comment on attachment 8587420 [details] [diff] [review]
Add a menucaption element

>diff --git a/toolkit/themes/windows/global/menu.css b/toolkit/themes/windows/global/menu.css
I can't speak for the other themes, but our menu binding story is a mess.
We have a binding for menubar menus (and a separate binding for menubar menus with icons, although I don't think anybody uses them).
We have a binding for menupopup menus and menuitems, and a separate binding if they have an icon, a checkbox or a radio. Because those take up space, the regular binding for menuitems has different CSS applied to it to shift the menutext across.
We then use the icon binding for menuitems in menulists, but hide the area reserved for the icon.
Because you based the menucaption on the regular menupopup menuitem, it's getting native appearance which gives it an extra left margin.
Changing the menucaption's label to the iconic class seems to fix that margin but of course it makes the other styles not apply and I thought I might as well just give you f+ on the patch so far.
Attachment #8587420 - Attachment is patch: true
Attachment #8587420 - Flags: review?(neil) → feedback+
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Blocks: e10s-select
Hi Neil, I was going through M6 bugs. What's the status here?
Flags: needinfo?(enndeakin)
We just tested this on w3c schools and can't reproduce. There is a corner case where the index can be off and we select an opt group on first display, we're going to file a new bug on that.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
Ok, after doing some more testing, we found this bug does exist when navigating using the keyboard. Reopening. However we do not think this blocks m6.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I think I should ask Neil what he thinks should be done here, based on comment 17. We can file a separate bug on cleanup of the menu bindings and theme.

I can make the menulist > menupopup > menucaption binding be closer to the iconic one. Or is there more?
Flags: needinfo?(enndeakin) → needinfo?(neil)
Ah, yes, I forgot to mention the resulting issue. At least on my Windows build, I get this display:
     RightJS
    RightJS 2.3.1
    RightJS 2.1.1
     Three.js
    Three.js r54
     Zepto
    Zepto 1.0rc1
     Enyo
etc. when the captions should obviously have no indentation, rather than extra indentation.
Flags: needinfo?(neil)
Posted patch Add a menucaption element, v2 (obsolete) — Splinter Review
Attachment #8587420 - Attachment is obsolete: true
Attachment #8598127 - Attachment is obsolete: true
Attachment #8598602 - Flags: review?(neil)
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Iteration: 40.3 - 11 May → 41.1 - May 25
Neil, would you be able to review this again soon?
Flags: needinfo?(neil)
Comment on attachment 8598602 [details] [diff] [review]
Add a menucaption element, v2.1

Sorry for the delay. This version seems to be fine, at least on Windows.
Flags: needinfo?(neil)
Attachment #8598602 - Flags: review?(neil) → review+
Duplicate of this bug: 1170829
https://hg.mozilla.org/mozilla-central/rev/9c8afe52c600
https://hg.mozilla.org/mozilla-central/rev/45f0d3729d5f
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
I get this error:

Warning: Unknown pseudo-class or pseudo-element 'moz-any('.  Ruleset ignored due to bad selector.
Source file: chrome://global/skin/menu.css
Line: 240, Column: 24

Correct should be ':-moz-any()'
Depends on: 1172282
QA Whiteboard: [good first verify][verify in Nightly only]
Reproduce on Firefox nightly 34.0a1 (2014-08-01) with windows 7 64 bit

The bug's fix is verified on latest Nightly 43.0a1 (Build ID : 20150820030202)
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
Whiteboard: [testday-20150821]
Verified as fixed on latest aurora 42.0a2 (Build ID 20150911004112; User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0)

and also on latest nightly 43.0a1 (Build ID 20150911030204; User Agent Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0)

changing status resolved fixed to verified fixed.
[Testday-20150911]
Status: RESOLVED → VERIFIED
Reproduce on Firefox nightly 34.0a1 (2014-08-01) on ubuntu 14.04 LTS, 32-bit!

The bug's fix is verified on Nightly 44.0a1!

Build ID: 20151001030236
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20151002]
You need to log in before you can comment on or make changes to this bug.