Closed
Bug 1387697
Opened 7 years ago
Closed 7 years ago
UITour won't highlight pocket correctly anymore now that it's moved to the page action menu
Categories
(Firefox :: Tours, enhancement, P1)
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.
Reporter | ||
Comment 1•7 years ago
|
||
This isn't failing in tests because the page action item is preffed off in tests.
Comment 2•7 years ago
|
||
(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.
Updated•7 years ago
|
Whiteboard: [photon-structure][triage][photon-onboarding][triage] → [photon-onboarding][triage]
Comment 3•7 years ago
|
||
remove whiteboard tag based on triage result
Whiteboard: [photon-onboarding][triage]
Assignee | ||
Comment 4•7 years ago
|
||
(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)
Reporter | ||
Comment 5•7 years ago
|
||
(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)
Assignee | ||
Comment 6•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
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?
Assignee | ||
Comment 11•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
(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
Updated•7 years ago
|
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Whiteboard: [photon-onboarding]
Reporter | ||
Comment 15•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Assignee | ||
Comment 20•7 years ago
|
||
Assignee | ||
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 24•7 years ago
|
||
(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)
Reporter | ||
Comment 25•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 26•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 29•7 years ago
|
||
(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
Reporter | ||
Comment 30•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 31•7 years ago
|
||
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
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21236fb6e100
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 33•7 years ago
|
||
I have verified this is fixed on today's nightly.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•