optgroup in <select> should not be selectable. STR mouse up/down

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.

Move old M2's low-priority bugs to M6 milestone.

Neil, is this patch ready for review?

I'll update it and try it out.

This patch fixes some issues with the selected index when reopening and adds a test

Running the test causes a failure due to SelectParentHelper not being cleared.

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`?

Comment on attachment 8580015 [details] [diff] [review] Updated patch This adds a caption="true" for menuitems that are captions.

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.

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!

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)

(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.

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.

Hi Neil, I was going through M6 bugs. What's the status here?

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.

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.

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?

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.

Neil, would you be able to review this again soon?

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8afe52c600 https://hg.mozilla.org/integration/mozilla-inbound/rev/45f0d3729d5f

https://hg.mozilla.org/mozilla-central/rev/9c8afe52c600 https://hg.mozilla.org/mozilla-central/rev/45f0d3729d5f

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()'

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

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]

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]