1355322 - Add anchor button to urlbar for action menu

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement, P1)

Description

[Drew Willcoxon :adw]

It's the ellipsis thing in the mockups.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1355322 - Add anchor button to urlbar for action menu.

Review commit: https://reviewboard.mozilla.org/r/133192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133192/

Comment 2

[Drew Willcoxon :adw]

This needs an ellipsis image, but other than that it's ready for review.  Are there assets somewhere for everything we need?

Comment 3

[:Gijs (he/him)]

Attachment: Icon

Yeah, the assets are in a gdrive folder, here: https://drive.google.com/drive/folders/0B6F4iApRJzosVTdaZjN3UHY5NG8 . I think you should have access with a mozilla.com account. I've attached the "more" icon for this bit for simplicity.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

https://reviewboard.mozilla.org/r/133192/#review136074

This is a good start, but it looks like the icon is 16px, and I can't find a detailed spec for the spacing, so I guess it makes sense for me to take another look once the icon is swapped in (as it's a different size to the one you've used in this spec, and we won't need the hidpi code anymore as it's SVG).

::: browser/base/content/browser.xul:807
(Diff revision 1)
> +                <menupopup id="pageActionMenu">
> +                  <menuitem id="pageActionMenu0"
> +                            label="Page Action Menu 0"/>
> +                  <menuitem id="pageActionMenu1"
> +                            label="Page Action Menu 1"/>
> +                </menupopup>

I think we'll want to use a panel, given the design, probably with a panelmultiview. I expect we may need to set hidden=true on it to avoid the associated perf hit, and unhide it on first use.

::: browser/themes/linux/browser.css:1135
(Diff revision 1)
>      -moz-image-region: rect(56px, 56px, 84px, 28px);
>    }
>  }
>  
> +#urlbar-page-action-button {
> +  display: none;

Given that we're messing with this at runtime anyway, and that this isn't theme-specific, maybe we should just set the hidden attribute on the button and remove that attribute from the JS where you're currently setting display: -moz-box? Then until 57, it won't show up for people using custom themes, either, so that seems slightly better.

::: browser/themes/linux/browser.css:1138
(Diff revision 1)
>  
> +#urlbar-page-action-button {
> +  display: none;
> +  -moz-appearance: none;
> +  list-style-image: url("chrome://browser/skin/reload-stop-go.png");
> +  padding: 0 9px;

This horizontal spacing looks like a bit much to me, especially with the 16px icon, though there doesn't seem to be a concrete spec...

Comment 5

[Drew Willcoxon :adw]

(In reply to :Gijs from comment #3)
> I've attached the "more" icon for this bit for simplicity.

Thanks!

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133192/diff/1-2/

Comment 7

[Drew Willcoxon :adw]

The gPageActionButton object is based on the gIdentityHandler object right above it in the code.  Some of the lines in onEvent are verbatim.  I expect that it'll be fleshed out as the panel itself is implemented (bug 1355323).

Is this the usual way to write menu-type toolbar buttons that use fancy panels instead of menupopups with menuitems?  Writing an event handler to listen for clicks and keypresses and then opening the panel manually in JS?  You can't just set an attribute on the button that takes care of that?

Comment 8

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #7)
> The gPageActionButton object is based on the gIdentityHandler object right
> above it in the code.  Some of the lines in onEvent are verbatim.  I expect
> that it'll be fleshed out as the panel itself is implemented (bug 1355323).
> 
> Is this the usual way to write menu-type toolbar buttons that use fancy
> panels instead of menupopups with menuitems?  Writing an event handler to
> listen for clicks and keypresses and then opening the panel manually in JS? 
> You can't just set an attribute on the button that takes care of that?

type=menu toolbarbuttons that have a <panel> child will automatically open it (just like a <menupopup>). That's probably a better way of doing this, though I don't know offhand when you'd remove the hidden attribute from the panel (it doesn't work if the panel is hidden=true)... Maybe we should just leave off the hidden attribute if you decide to go this way and worry about that performance aspect if it actually becomes a problem. I'm sure we could teach the XUL popup manager to deal with that problem if necessary.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

https://reviewboard.mozilla.org/r/133192/#review136786

Few more notes, but this is almost done, I think. I commented on the bug about the panel handler. Up to you how you deal with that in the end, I can live with either what you have right now and potentially moving it to a nested <panel> with type=menu on the toolbarbutton (and hiding the dropmarker) or this.

::: browser/base/content/browser.js:7763
(Diff revision 2)
>    }
>  };
>  
> +
> +var gPageActionButton = {
> +

Nit: please remove the empty line at the start here (which I thought would set off eslint, but maybe it doesn't?)

::: browser/base/content/browser.xul:814
(Diff revision 2)
> +              <toolbarbutton id="urlbar-page-action-button"
> +                             hidden="true"

We'll need a tooltiptext or an aria-label attribute so that it's obvious to accessibility users what this button is. I think for now it can just say "Page Actions".

::: browser/themes/linux/browser.css:1138
(Diff revision 2)
>  
> +/* page action button in location bar */
> +
> +#urlbar-page-action-button {
> +  -moz-appearance: none;
> +  list-style-image: url("chrome://browser/skin/more-16.svg");

Hm, I wish there were a shared place we could put this...

Anyway, to make this work in dark lightweight themes (including the dark compact theme), can you add:

`fill: currentColor`

so we use the foreground colours for the icon even on dark themes? (If there isn't a good shared place, we'll want this in all the themes.)

::: browser/themes/shared/jar.inc.mn:148
(Diff revision 2)
>    skin/classic/browser/compacttheme/loading-inverted.png (../shared/compacttheme/loading-inverted.png)
>    skin/classic/browser/compacttheme/[e-mail] (../shared/compacttheme/[e-mail])
>    skin/classic/browser/compacttheme/urlbar-history-dropmarker.svg (../shared/compacttheme/urlbar-history-dropmarker.svg)
>    skin/classic/browser/urlbar-star.svg                         (../shared/urlbar-star.svg)
>    skin/classic/browser/urlbar-tab.svg                          (../shared/urlbar-tab.svg)
> +  skin/classic/browser/more-16.svg                             (../shared/more-16.svg)

Can we name the icon something more topical, like 'page-action-icon.svg' or something like that? Sorry, I was lazy and didn't rename it myself when copying the file out of the archive google drive created...

We can definitely drop the -16 suffix, especially because it's SVG.

::: browser/themes/shared/more-16.svg:1
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">

The files as supplied by UX unfortunately still need a license header. I also believe you can omit the viewBox attribute.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133192/diff/2-3/

Comment 11

[Drew Willcoxon :adw]

I went with the onclick handler plus gPageActionButton as we talked about today.

(In reply to :Gijs from comment #9)
> > +var gPageActionButton = {
> > +
> 
> Nit: please remove the empty line at the start here (which I thought would
> set off eslint, but maybe it doesn't?)

Fixed

> We'll need a tooltiptext or an aria-label attribute so that it's obvious to
> accessibility users what this button is. I think for now it can just say
> "Page Actions".

Added tooltiptext

> ::: browser/themes/linux/browser.css:1138
> (Diff revision 2)
> >  
> > +/* page action button in location bar */
> > +
> > +#urlbar-page-action-button {
> > +  -moz-appearance: none;
> > +  list-style-image: url("chrome://browser/skin/more-16.svg");
> 
> Hm, I wish there were a shared place we could put this...

Good point, I moved all the CSS to browser/themes/shared/browser.inc.css.

> Anyway, to make this work in dark lightweight themes (including the dark
> compact theme), can you add:
> 
> `fill: currentColor`

Fixed

> ::: browser/themes/shared/jar.inc.mn:148
> (Diff revision 2)
> >    skin/classic/browser/compacttheme/loading-inverted.png (../shared/compacttheme/loading-inverted.png)
> >    skin/classic/browser/compacttheme/[e-mail] (../shared/compacttheme/[e-mail])
> >    skin/classic/browser/compacttheme/urlbar-history-dropmarker.svg (../shared/compacttheme/urlbar-history-dropmarker.svg)
> >    skin/classic/browser/urlbar-star.svg                         (../shared/urlbar-star.svg)
> >    skin/classic/browser/urlbar-tab.svg                          (../shared/urlbar-tab.svg)
> > +  skin/classic/browser/more-16.svg                             (../shared/more-16.svg)
> 
> Can we name the icon something more topical, like 'page-action-icon.svg' or
> something like that? Sorry, I was lazy and didn't rename it myself when
> copying the file out of the archive google drive created...

page-action.svg seems sufficient, so I chose that.

> ::: browser/themes/shared/more-16.svg:1
> (Diff revision 2)
> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
> 
> The files as supplied by UX unfortunately still need a license header. I
> also believe you can omit the viewBox attribute.

Fixed

Comment 12

[:Gijs (he/him)]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

https://reviewboard.mozilla.org/r/133192/#review137244

Looks great, thanks! Just one comment below about the tooltip string.

::: browser/base/content/browser.xul:817
(Diff revision 3)
>                               command="Browser:Stop"
>                               tooltip="dynamic-shortcut-tooltip"/>
> +              <toolbarbutton id="urlbar-page-action-button"
> +                             hidden="true"
> +                             class="chromeclass-toolbar-additional"
> +                             tooltiptext="Page Actions"

This should be a localizable string in browser.dtd (or some other dtd we can use from here). We're going to get rid of the "This space intentionally left blank" bit in the panel, so that bit is fine I think, but this is actually permanent, so we should make sure it can be localized.

Comment 13

[Drew Willcoxon :adw]

Ah yeah, that was dumb of me.

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133192/diff/3-4/

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c2e1aaed81fb -d e82ca7f0565b: rebasing 392294:c2e1aaed81fb "Bug 1355322 - Add anchor button to urlbar for action menu. r=Gijs" (tip)
merging browser/base/content/browser.js
merging browser/base/content/browser.xul
merging browser/locales/en-US/chrome/browser/browser.dtd
merging browser/themes/shared/jar.inc.mn
warning: conflicts while merging browser/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8861223 [details]
Bug 1355322 - Add anchor button to urlbar for action menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133192/diff/4-5/

Comment 17

[Drew Willcoxon :adw]

Rebased on a fresh tree.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd2450cc9ba5
Add anchor button to urlbar for action menu. r=Gijs

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd2450cc9ba5

Comment 20

[u554753]

Verified on Windows 7, 10, Mac, and Ubuntu.

Comment 21

[Francesco Lodolo [:flod]]

(In reply to :Gijs from comment #12)
> This should be a localizable string in browser.dtd (or some other dtd we can
> use from here). We're going to get rid of the "This space intentionally left
> blank" bit in the panel, so that bit is fine I think, but this is actually
> permanent, so we should make sure it can be localized.

This was never fixed, filing a follow-up.

Comment 22

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #21)
> (In reply to :Gijs from comment #12)
> > This should be a localizable string in browser.dtd (or some other dtd we can
> > use from here). We're going to get rid of the "This space intentionally left
> > blank" bit in the panel, so that bit is fine I think, but this is actually
> > permanent, so we should make sure it can be localized.
> 
> This was never fixed, filing a follow-up.

Yes it was... that comment specifically refers to:

> +                             tooltiptext="Page Actions"

which is now localizable. It also specifically says that we don't need the dummy panel text to be localizable because it's just filler text that we will never ship and users will never see.

Comment 23

[Francesco Lodolo [:flod]]

(In reply to :Gijs from comment #22)
> Yes it was... that comment specifically refers to:
> 
> > +                             tooltiptext="Page Actions"
> 
> which is now localizable. It also specifically says that we don't need the
> dummy panel text to be localizable because it's just filler text that we
> will never ship and users will never see.

Uh, sorry, I misread both sentences as referring to the same thing.

It might not ship to users, but it's currently visible in Nightly (where localizers are working). Is there a bug tracking the removal of this?

Comment 24

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #23)
> (In reply to :Gijs from comment #22)
> > Yes it was... that comment specifically refers to:
> > 
> > > +                             tooltiptext="Page Actions"
> > 
> > which is now localizable. It also specifically says that we don't need the
> > dummy panel text to be localizable because it's just filler text that we
> > will never ship and users will never see.
> 
> Uh, sorry, I misread both sentences as referring to the same thing.
> 
> It might not ship to users, but it's currently visible in Nightly (where
> localizers are working). Is there a bug tracking the removal of this?

Oh, ugh, this is a consequence of Dão moving this panel from being behind a pref to being behind a build-time define, which is nightly-only. I hadn't considered that aspect of it when Dão asked me about the build-time define, though I'm still not terribly happy about that for other reasons...

In any case, the bug you want is bug 1355323.

Comment 25

[Alex Thayer [:alexical] (she/her)]

Not sure if this is the right place to put this, but I figured I should mention it. The onclick handler feels a little weird when I interact with this. Since the hamburger menu shows on mouse down, this feels delayed being on click. I'm not sure which is right, but it seems important to be consistent. Did you guys have a conversation about using onclick instead of mousedown? I see onclick being alluded to in comment #11.

Comment 26

[:Gijs (he/him)]

(In reply to Doug Thayer [:dthayer] from comment #25)
> Not sure if this is the right place to put this, but I figured I should
> mention it. The onclick handler feels a little weird when I interact with
> this. Since the hamburger menu shows on mouse down, this feels delayed being
> on click. I'm not sure which is right, but it seems important to be
> consistent. Did you guys have a conversation about using onclick instead of
> mousedown? I see onclick being alluded to in comment #11.

I think this is effectively bug 971260. We still don't allow for mouseup on the main panel menu to actually activate items, and we don't open any other panels with mousedown, including e.g. the identity panel (which is tricky anyway because it's draggable) and the downloads panel or any of the buttons that are in the palette/panel by default, if you move those to the main toolbar. We could argue that we'd want to implement support for the mousedown+mouseup flow for both the page action panel and the new hamburger panel (but potentially punt on "all panels" because of how much work it is) as part of Photon, perhaps in lieu of bug 1366074 -- but getting this right might be quite tricky. Philipp/Bryan, thoughts? Perhaps this could be a perceived-performance thing for the photon-perf team...

Comment 27

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

This sounds like a separate project.
The way we handle interactions on toolbar buttons is extremely inconsistent right now. The hamburger menu and the bookmarks menu use mousedown, most other things use mouseup.
Generally, using mousedown gets us a perceived perf win, but we probably shouldn't use it for actions, since it can't be cancelled.

I filed bug 1369693 for this, but it's probably out of scope for Photon.