1387697 - UITour won't highlight pocket correctly anymore now that it's moved to the page action menu

Status: VERIFIED FIXED

(Firefox :: Tours, enhancement, P1)

Description

[:Gijs (he/him)]

https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/uitour/UITour.jsm#189-193

We should make this return the url bar button if present, and the page action item otherwise.

Comment 1

[:Gijs (he/him)]

This isn't failing in tests because the page action item is preffed off in tests.

Comment 2

[Drew Willcoxon :adw]

(In reply to :Gijs from comment #1)
> This isn't failing in tests because the page action item is preffed off in
> tests.

And just to add a little more (maybe useless) context, that pref (extensions.pocket.disablePageAction) was only added as a temporary fix to keep Pocket-related tests passing until bug 1385418 is fixed.

Comment 3

[Francis Lee [:frlee]]

remove whiteboard tag based on triage result

Comment 4

[Fischer [:Fischer]]

(In reply to :Gijs from comment #0)
> https://dxr.mozilla.org/mozilla-central/rev/
> 52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/uitour/UITour.
> jsm#189-193
> 
> We should make this return the url bar button if present, and the page action item otherwise.
Hi Gijs,

Sounds like what we have to do is:
- Return the #pageAction-urlbar-pocket button if found, therwise return the #pageAction-panel-pocket button in the UITour.jsm
- Update browser_UITour_availableTargets.js as well

Before proceeding, I would like to check with you if it is ok to fix the bug right now?
From the comment 2, I could find `user_pref("extensions.pocket.disablePageAction", true);` still in the Central right now.

BTW, how is the priority of this bug?

[1] https://dxr.mozilla.org/mozilla-central/rev/a921bfb8a2cf3db4d9edebe9b35799a3f9d035da/testing/profiles/prefs_general.js#400

Thanks

Comment 5

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #4)
> (In reply to :Gijs from comment #0)
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/uitour/UITour.
> > jsm#189-193
> > 
> > We should make this return the url bar button if present, and the page action item otherwise.
> Hi Gijs,
> 
> Sounds like what we have to do is:
> - Return the #pageAction-urlbar-pocket button if found, therwise return the
> #pageAction-panel-pocket button in the UITour.jsm
> - Update browser_UITour_availableTargets.js as well

Yep, though you may need to flip the pref you mentioned later on in the test.

> Before proceeding, I would like to check with you if it is ok to fix the bug
> right now?

Yes, please feel free to submit a patch.

> From the comment 2, I could find
> `user_pref("extensions.pocket.disablePageAction", true);` still in the
> Central right now.

The page action is disabled (and thereby the toolbar button enabled) in tests right now, but only in tests - normal users will get the page action button, not the toolbar button.

> BTW, how is the priority of this bug?

Up to you. For our team it doesn't matter very much, but I don't know what plans the onboarding team has.

Comment 6

[Fischer [:Fischer]]

(In reply to :Gijs from comment #5)
> (In reply to Fischer [:Fischer] from comment #4)
> > (In reply to :Gijs from comment #0)
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > 52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/uitour/UITour.
> > > jsm#189-193
> > > 
> > > We should make this return the url bar button if present, and the page action item otherwise.
> > Hi Gijs,
> > 
> > Sounds like what we have to do is:
> > - Return the #pageAction-urlbar-pocket button if found, therwise return the
> > #pageAction-panel-pocket button in the UITour.jsm
> > - Update browser_UITour_availableTargets.js as well
> 
> Yep, though you may need to flip the pref you mentioned later on in the test.
> 
> > Before proceeding, I would like to check with you if it is ok to fix the bug
> > right now?
> 
> Yes, please feel free to submit a patch.
> 
> > From the comment 2, I could find
> > `user_pref("extensions.pocket.disablePageAction", true);` still in the
> > Central right now.
> 
> The page action is disabled (and thereby the toolbar button enabled) in
> tests right now, but only in tests - normal users will get the page action
> button, not the toolbar button.
> 
> > BTW, how is the priority of this bug?
> 
> Up to you. For our team it doesn't matter very much, but I don't know what plans the onboarding team has.
Thanks. Then probably we could fix this bug while modifying UITour btw.

Comment 7

[Fischer [:Fischer]]

Attachment: Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 8

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 9

[Drew Willcoxon :adw]

Fischer, I'm fixing this as part of bug 1385418 -- it doesn't really make sense to fix that bug without hooking up Pocket to UITour.  Sorry for not commenting on that sooner.  Is that OK?

Comment 11

[Fischer [:Fischer]]

(In reply to Drew Willcoxon :adw from comment #9)
> Fischer, I'm fixing this as part of bug 1385418 -- it doesn't really make
> sense to fix that bug without hooking up Pocket to UITour.  Sorry for not
> commenting on that sooner.  Is that OK?
Hi Drew,
Saw you are fixing bug 1390048 in bug 1385418. Thank you for doing that.
Basically the functionality this bug is adding doesn't depend on bug 1385418. The only thing is "extensions.pocket.disablePageAction" and is just a matter of removing that part in the test while rebasing.
Thus, probably we could proceed parallelly. Thank you.

Comment 12

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 13

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 14

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #13)
> Comment on attachment 8897326 [details]
> Bug 1387697 - Make UITour locate page actions on the urlbar first then on
> the page action panel,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/168634/diff/3-4/
Hi Gijs,
This patch unified how UITour locates page action targets. Return button on the urlbar first if found, otherwise return button on the page action panel, thanks.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6651338362559aee4bc6a326fc2eda2257dbc89

Comment 15

[:Gijs (he/him)]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

https://reviewboard.mozilla.org/r/168634/#review174390

I think we should just alias the bookmark-star-button to the bookmark stuff, and add a test that that works, too.

::: browser/components/uitour/UITour-lib.js
(Diff revision 4)
>     * <li>accountStatus
>     * <li>addons
>     * <li>appMenu
>     * <li>backForward
>     * <li>bookmarks
> -   * <li>bookmark-star-button

This has compat consequences for places using UITour. Are we sure we don't just want to make this an alias for pageAction-panel-bookmark ?

::: browser/components/uitour/UITour.jsm:153
(Diff revision 4)
>      ["help",        {query: "#appMenu-help-button"}],
>      ["home",        {query: "#home-button"}],
>      ["library",     {query: "#appMenu-library-button"}],
>      ["pocket", {
>        allowAdd: true,
> -      query: "#pocket-button",
> +      query: (aDocument) => {

This will conflict with bug 1385418.

::: browser/components/uitour/test/head.js:370
(Diff revision 4)
>    Services.prefs.setBoolPref("browser.uitour.enabled", true);
>    let testHttpsUri = Services.io.newURI("https://example.org");
>    let testHttpUri = Services.io.newURI("http://example.org");
>    Services.perms.add(testHttpsUri, "uitour", Services.perms.ALLOW_ACTION);
>    Services.perms.add(testHttpUri, "uitour", Services.perms.ALLOW_ACTION);
> +  turnOnPcoketPageAction();

Spelling here and below. ("Pcoket")

::: browser/components/uitour/test/head.js:397
(Diff revision 4)
> +  // TODO: After the bug 1385418, should remove the code about Pocket page action
> +  Services.prefs.setBoolPref("extensions.pocket.disablePageAction", false);

Maybe just drop this already and plan to land after 1385418? Seems simpler.

Comment 16

[Fischer [:Fischer]]

Attachment: UITour_showHighlight_bookmarks.png

Comment 17

[Fischer [:Fischer]]

Attachment: UITour_showHighlight_bookmarks.png

Comment 18

[Fischer [:Fischer]]

Attachment: UITour_showHighlight_pageAction_bookmark.png

Comment 19

[Fischer [:Fischer]]

Attachment: urlbar_page_action_bookmark.png

Comment 20

[Fischer [:Fischer]]

Attachment: bookmark_star_button_dom_structure.png

Comment 21

[Fischer [:Fischer]]

The current code queries `#star-button` for the bookmark star button on the urlbar[1].
In this patch, we switches to `#star-button-box`.

The image#star-button is under hbox#star-button-box[2]. Using hbox#star-button-box is fine for UITour highlight too.
Why switching to hbox#star-button-box is for the convenience of checking the button hidden state as below
```
    ["pageAction-bookmark", {
      query: (aDocument) => {
        // The bookmark's urlbar page action button is pre-defined in the DOM.
        // It would be hidden if toggled off from the urlbar.
        let node = aDocument.getElementById("star-button-box");
        if (node && node.hidden == false) {
          return node;
        }
        return aDocument.getElementById("pageAction-panel-bookmark");
      },
    }],
```

[1] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/browser/components/uitour/UITour.jsm#224
[2] attachment 8898178 [details]: bookmark_star_button_dom_structure.png

Comment 22

[Fischer [:Fischer]]

(In reply to :Gijs from comment #15)
> Comment on attachment 8897326 [details]
> Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,
> 
> https://reviewboard.mozilla.org/r/168634/#review174390
> 
> I think we should just alias the bookmark-star-button to the bookmark stuff, and add a test that that works, too.
> 
> ::: browser/components/uitour/UITour-lib.js (Diff revision 4)
> >     * <li>bookmarks
> > -   * <li>bookmark-star-button
> 
> This has compat consequences for places using UITour. Are we sure we don't just want to make this an alias for pageAction-panel-bookmark ?
> 
Make sure I didn't get it wrong.
We make querying "pageAction-bookmark" return "bookmark-star-button" first then "pageAction-panel-bookmark".
- `Mozilla.UITour.showHighligh("bookmars")` would highlight the button opening the bookmarks menu. See [1].
- `Mozilla.UITour.showHighligh("pageAction-bookmark")` would highlight the button bookmarking a page if existed. See [2][3].

The "bookmark-star-button" is the "pageAction-panel-bookmark" button added to the urlbar so we treat it like other page action items.
Does this make sense?

p.s we did some changes on querying target, please see comment 21 for the reason.

[1] attachment 8898170 [details]: UITour_showHighlight_bookmarks.png
[2] attachment 8898171 [details]: UITour_showHighlight_pageAction_bookmark.png
[3] attachment 8898175 [details]: urlbar_page_action_bookmark.png

> ::: browser/components/uitour/UITour.jsm:153 (Diff revision 4)
> > -      query: "#pocket-button",
> > +      query: (aDocument) => {
> 
> This will conflict with bug 1385418.
> 
Thanks, rebased on the bug 1385418

> ::: browser/components/uitour/test/head.js:370
> (Diff revision 4)
> >    Services.prefs.setBoolPref("browser.uitour.enabled", true);
> >    let testHttpsUri = Services.io.newURI("https://example.org");
> >    let testHttpUri = Services.io.newURI("http://example.org");
> >    Services.perms.add(testHttpsUri, "uitour", Services.perms.ALLOW_ACTION);
> >    Services.perms.add(testHttpUri, "uitour", Services.perms.ALLOW_ACTION);
> > +  turnOnPcoketPageAction();
> 
> Spelling here and below. ("Pcoket")
> 
Thanks for pointing this out.

> ::: browser/components/uitour/test/head.js:397
> (Diff revision 4)
> > +  // TODO: After the bug 1385418, should remove the code about Pocket page action
> > +  Services.prefs.setBoolPref("extensions.pocket.disablePageAction", false);
> 
> Maybe just drop this already and plan to land after 1385418? Seems simpler.
Right, rebased on the bug 1385418 and removed this.

Comment 23

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 24

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #22)
> (In reply to :Gijs from comment #15)
> > Comment on attachment 8897326 [details]
> > Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,
> > 
> > https://reviewboard.mozilla.org/r/168634/#review174390
> > 
> > I think we should just alias the bookmark-star-button to the bookmark stuff, and add a test that that works, too.
> > 
> > ::: browser/components/uitour/UITour-lib.js (Diff revision 4)
> > >     * <li>bookmarks
> > > -   * <li>bookmark-star-button
> > 
> > This has compat consequences for places using UITour. Are we sure we don't just want to make this an alias for pageAction-panel-bookmark ?
> > 
> Make sure I didn't get it wrong.
> We make querying "pageAction-bookmark" return "bookmark-star-button" first
> then "pageAction-panel-bookmark".
> - `Mozilla.UITour.showHighligh("bookmars")` would highlight the button
> opening the bookmarks menu. See [1].
> - `Mozilla.UITour.showHighligh("pageAction-bookmark")` would highlight the
> button bookmarking a page if existed. See [2][3].
> 
> The "bookmark-star-button" is the "pageAction-panel-bookmark" button added
> to the urlbar so we treat it like other page action items.
> Does this make sense?
> 
> p.s we did some changes on querying target, please see comment 21 for the
> reason.

No, I meant it seems to me that:

Mozilla.UITour.showHighligh("bookmark-star-button")

should continue to work and highlight the star button or, if it's not in the url bar, the bookmark item in the page action menu, on builds after your change. Your commit seems to just remove support for highlighting the bookmark-star-button. If you've discussed this with the web folks who call these APIs and all their pages are updated, I guess we can leave it like that, but generally it seems like backward compat is cheap here so it's not clear to me why you abandoned it. :-)

Comment 25

[:Gijs (he/him)]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

https://reviewboard.mozilla.org/r/168634/#review174916

Clearing review pending discussion of the fate of bookmark-star-button

Comment 26

[Fischer [:Fischer]]

(In reply to :Gijs from comment #24)
> (In reply to Fischer [:Fischer] from comment #22)
> > The "bookmark-star-button" is the "pageAction-panel-bookmark" button added
> > to the urlbar so we treat it like other page action items.
> > Does this make sense?
> > 
> > p.s we did some changes on querying target, please see comment 21 for the
> > reason.
> 
> No, I meant it seems to me that:
> 
> Mozilla.UITour.showHighligh("bookmark-star-button")
> 
> should continue to work and highlight the star button or, if it's not in the
> url bar, the bookmark item in the page action menu, on builds after your
> change. Your commit seems to just remove support for highlighting the
> bookmark-star-button. If you've discussed this with the web folks who call
> these APIs and all their pages are updated, I guess we can leave it like
> that, but generally it seems like backward compat is cheap here so it's not
> clear to me why you abandoned it. :-)
Thanks for reminding. Yes the compat is always the thing to take care. The reason we just remove the support of "bookmark-star-button" is because it was added in the bug 1382579 only got checked-in around 10 days ago. Back then when writing the bug 1382579 patch, the bug 1374477 was a bit undergoing as well and hadn't looked into this bug so didn't do the consideration in this bug.


(In reply to :Gijs from comment #25)
> Comment on attachment 8897326 [details]
> Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel
> 
> https://reviewboard.mozilla.org/r/168634/#review174916
> 
> Clearing review pending discussion of the fate of bookmark-star-button
Sorry, I cleared the "r?Gijs" in the commit msg then updated MozReview. I planed to send out r? after our ni? discussion. I should learn more about MozReview.

Comment 27

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

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

Comment 28

[Fischer [:Fischer]]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

https://reviewboard.mozilla.org/r/168634/#review175182

::: browser/components/uitour/UITour.jsm:224
(Diff revision 6)
>      }],
> -    ["pageAction-panel-bookmark", {
> -      query: "#pageAction-panel-bookmark"
> -    }],
> -    ["pageAction-panel-copyURL", {
> -      query: "#pageAction-panel-copyURL"
> +    ["pageAction-bookmark", {
> +      query: (aDocument) => {
> +        // The bookmark's urlbar page action button is pre-defined in the DOM.
> +        // It would be hidden if toggled off from the urlbar.
> +        let node = aDocument.getElementById("star-button-box");

The old code queries `#star-button` for the bookmark star button on the urlbar.
In this patch, we switches to `#star-button-box`.
The image#star-button is under hbox#star-button-box. Using hbox#star-button-box is fine for UITour highlight too.
Why switching to hbox#star-button-box is for the convenience of checking the button hidden state

::: browser/components/uitour/UITour.jsm:243
(Diff revision 6)
> -      query: "#pageAction-panel-sendToDevice"
> +      query: (aDocument) => {
> +        return aDocument.getElementById("pageAction-urlbar-emailLink") ||
> +               aDocument.getElementById("pageAction-panel-emailLink");
> +      },
>      }],
> -    ["bookmark-star-button", {
> +    ["pageAction-sendToDevice", {

The "bookmark-star-button" is the urlbar button of the bookmark page action so is moved into the "pageAction-bookmark". It was added btw in the bug bug 1382579 around 10 days ago. No one is using it so fine to make this change.

Comment 29

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #27)
> Comment on attachment 8897326 [details]
> Bug 1387697 - Make UITour locate page actions on the urlbar first then on
> the page action panel,
> 
> Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168634/diff/5-6/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8795add571d19e81b22f39643e745cbcccdb0cd0

Comment 30

[:Gijs (he/him)]

Comment on attachment 8897326 [details]
Bug 1387697 - Make UITour locate page actions on the urlbar first then on the page action panel,

https://reviewboard.mozilla.org/r/168634/#review175226

Nice, thanks!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21236fb6e100
Make UITour locate page actions on the urlbar first then on the page action panel, r=Gijs

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21236fb6e100

Comment 33

[Justin [:JW_SoftvisionQA]]

I have verified this is fixed on today's nightly.

Comment 34

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed on Fx 57.0b7 as well.