Closed Bug 1387697 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Tours, enhancement, P1)

55 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(5 files, 1 obsolete file)

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.
This isn't failing in tests because the page action item is preffed off in tests.
(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.
Whiteboard: [photon-structure][triage][photon-onboarding][triage] → [photon-onboarding][triage]
remove whiteboard tag based on triage result
Whiteboard: [photon-onboarding][triage]
(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
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(fliu)
(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.
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?
Duplicate of this bug: 1390048
Depends on: 1385418
(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.
Flags: needinfo?(fliu)
(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
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Whiteboard: [photon-onboarding]
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.
Attachment #8897326 - Flags: review?(gijskruitbosch+bugs)
Attached image UITour_showHighlight_bookmarks.png (obsolete) —
Attachment #8898167 - Attachment is obsolete: true
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
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
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
Attachment #8897326 - Flags: review?(gijskruitbosch+bugs)
(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 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.
(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 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!
Attachment #8897326 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21236fb6e100
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have verified this is fixed on today's nightly.
Status: RESOLVED → VERIFIED
I can confirm this issue is fixed on Fx 57.0b7 as well.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.