890476 - Don't build the menubar on OS X

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Because, you know, it's not like there's a point.

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=322b50e8307d

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 771634 [details] [diff] [review]
Patch

If customizable="true" is missing, toolkit/content/widgets/toolbar.xml will be used, right? And that isn't going to care about whether that attribute is present or not. I don't think we want that.

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 771634 [details] [diff] [review]
> Patch
> 
> If customizable="true" is missing, toolkit/content/widgets/toolbar.xml will
> be used, right? And that isn't going to care about whether that attribute is
> present or not. I don't think we want that.

Why not?

Comment 4

[Dão Gottwald [:dao]]

Because that would read the currentset attribute and whatnot. We provide no UI that would set that attribute on OS X, but add-ons could do it or people could move profiles from different OSes.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Because that would read the currentset attribute and whatnot. We provide no
> UI that would set that attribute on OS X, but add-ons could do it or people
> could move profiles from different OSes.

I'm not sure that's something we should worry about. In any case, I'm not sure I see a much better alternative. We could build another shim XBL binding that essentially no-ops everything, but that is also liable to break add-ons because obviously things like insertItem/currentset wouldn't behave as they normally would. Would you be OK with that instead?

Comment 6

[Dão Gottwald [:dao]]

I don't think we want insertItem/currentSet (the property)/currentset (the attribute) to behave like they normally would, as this would move items into a place where users could neither see nor handle them. Whether that's best prevented by teaching the current binding about it or by adding a new one, I don't know.

Note that using toolkit/content/widgets/toolbar.xml would also mess with the toolbarpalette, for instance. This may be compatible with what browser/components/customizableui/content/toolbar.xml is doing, but it seems super fragile. I really don't think letting two different toolbar customization implementations run side by side is the way to go.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #6)
> I don't think we want insertItem/currentSet (the property)/currentset (the
> attribute) to behave like they normally would, as this would move items into
> a place where users could neither see nor handle them. Whether that's best
> prevented by teaching the current binding about it or by adding a new one, I
> don't know.
> 
> Note that using toolkit/content/widgets/toolbar.xml would also mess with the
> toolbarpalette, for instance. This may be compatible with what
> browser/components/customizableui/content/toolbar.xml is doing, but it seems
> super fragile. I really don't think letting two different toolbar
> customization implementations run side by side is the way to go.

OK. I'll go with a new one just because all the #ifdef XP_MACOSX and/or other quick returns in the middle of methods would get really annoying... If we're essentially stubbing everything it shouldn't really be a huge binding anyway.

Comment 8

[:Gijs (he/him)]

Attachment: Don't build a menubar on OSX

So, something like this? Pushed to try (forgot platform, oops...): https://tbpl.mozilla.org/?tree=Try&rev=e491eba53868

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 771687 [details] [diff] [review]
Don't build a menubar on OSX

>+    <resources>
>+      <stylesheet src="chrome://global/skin/toolbar.css"/>
>+    </resources>

What is this good for?

>+    <implementation implements="nsIAccessibleProvider">
>+      <property name="accessibleType" readonly="true">
>+        <getter>
>+          return Components.interfaces.nsIAccessibleProvider.XULToolbar;
>+        </getter>
>+      </property>

And this?

>+      <property name="toolbarName"
>+                onget="return this.getAttribute('toolbarname');"
>+                onset="this.setAttribute('toolbarname', val); return val;"/>

And this?

>+      <property name="currentSet" readonly="true">
>+        <getter><![CDATA[
>+          return [node.id for (node of this.children)].join(',');

return this.getAttribute("defaultset");?

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 771687 [details] [diff] [review]
> Don't build a menubar on OSX
> 
> >+    <resources>
> >+      <stylesheet src="chrome://global/skin/toolbar.css"/>
> >+    </resources>
> 
> What is this good for?

I reused it in a "let's change as little as necessary" mood, but looking at the rules, I guess this can go.

> 
> >+    <implementation implements="nsIAccessibleProvider">
> >+      <property name="accessibleType" readonly="true">
> >+        <getter>
> >+          return Components.interfaces.nsIAccessibleProvider.XULToolbar;
> >+        </getter>
> >+      </property>
> 
> And this?

This I'm not sure about. The toolbar has height 0, but is otherwise 'visible'; I don't know what the Mac a11y APIs do in this case, and if we need to expose anything or if nothing at all will do or... so I left it how it was. If you think it's necessary, we can ask someone from the a11y team.

> >+      <property name="toolbarName"
> >+                onget="return this.getAttribute('toolbarname');"
> >+                onset="this.setAttribute('toolbarname', val); return val;"/>
> 
> And this?

Do we not use this anywhere but in the contextmenu? If so, it can go, too.

> >+      <property name="currentSet" readonly="true">
> >+        <getter><![CDATA[
> >+          return [node.id for (node of this.children)].join(',');
> 
> return this.getAttribute("defaultset");?

Yes, I suppose that would work too.

Comment 11

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #10)
> > >+    <implementation implements="nsIAccessibleProvider">
> > >+      <property name="accessibleType" readonly="true">
> > >+        <getter>
> > >+          return Components.interfaces.nsIAccessibleProvider.XULToolbar;
> > >+        </getter>
> > >+      </property>
> > 
> > And this?
> 
> This I'm not sure about. The toolbar has height 0, but is otherwise
> 'visible'; I don't know what the Mac a11y APIs do in this case, and if we
> need to expose anything or if nothing at all will do or... so I left it how
> it was. If you think it's necessary, we can ask someone from the a11y team.

I don't see what this could be good for, but feel free to ask them.

> > >+      <property name="toolbarName"
> > >+                onget="return this.getAttribute('toolbarname');"
> > >+                onset="this.setAttribute('toolbarname', val); return val;"/>
> > 
> > And this?
> 
> Do we not use this anywhere but in the contextmenu? If so, it can go, too.

This isn't even accessed for the context menu, only the attribute is used there.

Comment 12

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > > >+    <implementation implements="nsIAccessibleProvider">
> > > >+      <property name="accessibleType" readonly="true">
> > > >+        <getter>
> > > >+          return Components.interfaces.nsIAccessibleProvider.XULToolbar;
> > > >+        </getter>
> > > >+      </property>
> > > 
> > > And this?
> > 
> > This I'm not sure about. The toolbar has height 0, but is otherwise
> > 'visible'; I don't know what the Mac a11y APIs do in this case, and if we
> > need to expose anything or if nothing at all will do or... so I left it how
> > it was. If you think it's necessary, we can ask someone from the a11y team.
> 
> I don't see what this could be good for, but feel free to ask them.

I'd be surprised if the current value was correct, but have no idea what the correct value is here.

Marco, what's the best thing to do for an essentially invisible-but-not-really XUL toolbar element used only to contain the menubar - on OS X? Can we just remove the accessibleType entirely, do we need role="presentation", is the a11y code smart enough to ignore it anyway (presumably OS X takes care of the a11y of the global menubar?), ...? What's the way to go?

(we currently return Ci.nsIAccessibleProvider.XULToolbar for all toolbars, including the menubar, but we're now special-casing the menubar's toolbar on OS X)

Comment 13

[alexander :surkov (:asurkov)]

in general a11y doesn't need invisible things at least until they communicate to the user somehow (example is a chat in the background tab). Technically we don't create an accessible object if its DOM element doesn't have a frame object. If 0 height toolbar doesn't have frame then there's no accessible and you shouldn't worry about nsIAccesibleProvider value. 

Anyway the best way it's just get Marco to take a look, he probably will need a build containing the change.

But what is idea of invisible toolbar of 0 height used to contain a OS X menubar?

Comment 14

[:Gijs (he/him)]

Attachment: Patch v1.1

From my testing, this works; The toolbar no longer gets an accessible in the a11y tree in DOMI, but seeing as there's nothing useful in it I think that's OK (it didn't have an accessible name before anyway) and menus continue to work fine in VoiceOver, as far as I can tell, so this should be OK.

Comment 15

[Marco Zehe (:MarcoZ)]

I think you should be good with the changes you've made, Gijs. If you like, you can throw a try build at me, but your testing sounds thorough to me.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 772039 [details] [diff] [review]
Patch v1.1

nit: I'd prefer "stub" over "noop" since the binding isn't really a no-op.

Comment 17

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 772039 [details] [diff] [review]
> Patch v1.1
> 
> nit: I'd prefer "stub" over "noop" since the binding isn't really a no-op.

Good point. Fixed, and pushed: https://hg.mozilla.org/projects/ux/rev/34c0f40827de

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/34c0f40827de