Closed
Bug 1121210
Opened 9 years ago
Closed 9 years ago
Loop: info panel position breaks when "Contacts" tab is activated in Hello panel
Categories
(Firefox :: Tours, defect)
Firefox
Tours
Tracking
()
People
(Reporter: cstipkovic, Assigned: mikedeboer)
References
Details
(Whiteboard: [UITour:P2])
Attachments
(4 files, 4 obsolete files)
2.29 MB,
image/png
|
Details | |
2.35 MB,
image/png
|
Details | |
22.29 KB,
patch
|
MattN
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
57.35 KB,
image/jpeg
|
Details |
Steps to reproduce: 1. Click on the Firefox Hello icon; 2. Click on the Contacts icon; This happens just when the "Welcome" box is on the screen.
Reporter | ||
Comment 1•9 years ago
|
||
Updated•9 years ago
|
Component: Client → Tours
OS: Mac OS X → All
Product: Loop → Firefox
Hardware: x86 → All
Summary: Welcome box layout breaks when contacts tab activated → Loop: info panel position breaks when "Contacts" tab is activated in Hello panel
Comment 2•9 years ago
|
||
Note that this can only happen if the user signed in with FxA otherwise the contact tab doesn't show.
Mentor: felipc, reuben.bmo
Updated•9 years ago
|
Blocks: fx-UITour-Hello
Reporter | ||
Comment 3•9 years ago
|
||
[:MattN], yes, you're right, I forgot to mention that I'm using the FxA. Thanks [:ckprice] and [:MattN] for improve this bug description :)
Updated•9 years ago
|
Blocks: fx-UITour-hello-36
Comment 5•9 years ago
|
||
Marking this as a P2 for Hello 36 tours. Although it will not impede development, we're at risk for a much larger audience being exposed to this UX. If the larger platform fix can't be implemented, let's see if we can execute a smaller scale fix, e.g. notifying web that the tab has changed.
Whiteboard: [UITour:P2]
Comment 6•9 years ago
|
||
(In reply to Clauber Stipkovic [:cstipkovic] from comment #3) > Thanks [:ckprice] and [:MattN] for improve this bug description :) Thank you for bringing this to our attention! It is very much appreciated.
Comment 7•9 years ago
|
||
Matt: I think you said you had an idea for how to workaround this specific case without the general fix in bug 1109868? What was it? Something about adding a hack to hire/reposition the infopanel when the contacts tab was clicked? I think we also talked about me being confused what's triggering this bug -- I thought panels got positioned when first shown, and stayed put... What's causing it to reposition when Contacts is clicked? Also, I noticed the panel will go back to the correct position when clicking the Conversations tab again, so I think that makes this less important to fix.
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 8•9 years ago
|
||
I'll implement a workaround with possibility to provide a secondary anchor, which will be used when the original anchor is hidden. So in this case that would be the conversations tab button.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Points: --- → 3
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 9•9 years ago
|
||
Or not. After some investigation, this bug is really caused by the panel implementation and bug 1109868 would fix this properly. I'll attach a patch that introduces the 'Loop:SelectedTabChanged' UITour notification, which the content page should act upon.
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8558507 -
Flags: feedback?(MattN+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8558507 -
Attachment description: Patch v1: notify UITour when the active tab changes and don't show the get started info panel when the contact tab is not selected → Patch v1: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected
Comment 11•9 years ago
|
||
Comment on attachment 8558507 [details] [diff] [review] Patch v1: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8558507 [details] [diff] [review]: ----------------------------------------------------------------- This approach may work but sometimes the panel gets in a broken state before the page has time to react. Alex is able to get the panel in a broken state but I'm not on my machine. Alex should try a try-build to confirm that this approach works. ::: browser/components/loop/content/js/panel.jsx @@ +37,5 @@ > > + shouldComponentUpdate: function(nextProps, nextState) { > + var tabChange = this.state.selectedTab !== nextState.selectedTab; > + if (tabChange) { > + this.props.mozLoop.notifyUITour("Loop:SelectedTabChanged"); s/Selected/Panel/ SelectedTab is too easy to confuse with tabbrowser tabs IMO and makes it more clear this is related to the panel instead of some other Loop UI. ::: browser/components/uitour/UITour.jsm @@ +148,5 @@ > // doesn't overlap the panel contents much. > return loopBrowser.contentDocument.querySelector(".new-room-button").parentElement; > }, > }], > ["loop-roomList", { loop-RoomList has the same problem I think
Attachment #8558507 -
Flags: feedback?(MattN+bmo) → feedback+
Comment 12•9 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #7) > Matt: I think you said you had an idea for how to workaround this specific > case without the general fix in bug 1109868? What was it? Something about > adding a hack to hire/reposition the infopanel when the contacts tab was > clicked? Yeah, either the approach that Mike has where we tell the page and it hides the highlight or we just listen for the tab change ourselves in UITour and hide. > I think we also talked about me being confused what's triggering this bug -- > I thought panels got positioned when first shown, and stayed put... What's > causing it to reposition when Contacts is clicked? It's layout/XUL code that makes the panel follow the anchor e.g. when you move the wrowser window. > Also, I noticed the panel will go back to the correct position when clicking > the Conversations tab again, so I think that makes this less important to > fix. Yeah, that can happen in some cases as long as the element is identical and hasn't been re-created to simply look the same.
Comment 13•9 years ago
|
||
Comment on attachment 8558507 [details] [diff] [review] Patch v1: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8558507 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/js/panel.jsx @@ +37,5 @@ > > + shouldComponentUpdate: function(nextProps, nextState) { > + var tabChange = this.state.selectedTab !== nextState.selectedTab; > + if (tabChange) { > + this.props.mozLoop.notifyUITour("Loop:SelectedTabChanged"); You should pass along the selectedTab name with notifyUITour so the page can handle switching back to the room tab if it wants.
Updated•9 years ago
|
status-firefox35:
--- → wontfix
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
Assignee | ||
Comment 14•9 years ago
|
||
Thanks for the feedback Matt! I updated the patch to incorporate your suggestions. Could you review those for me?
Attachment #8558507 -
Attachment is obsolete: true
Attachment #8560405 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8560405 [details] [diff] [review] Patch v2: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8560405 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/uitour/UITour.jsm @@ +84,5 @@ > +/** > + * Checks if a certain tab is selected inside the Loop panel. > + * > + * @param {XULElement} loopBrowser Reference to the Loop browser element > + * @param {String} tabName Name of the tab that's expected to be selected nit: I removed the superfluous space in the two lines above.
Comment 16•9 years ago
|
||
Comment on attachment 8560405 [details] [diff] [review] Patch v2: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8560405 [details] [diff] [review]: ----------------------------------------------------------------- I think the approach is good but I think there should be a test and the helpers should move to LoopUI (or somewhere else in Loop code). ::: browser/components/uitour/UITour.jsm @@ +76,5 @@ > + * > + * @param {XULDocument} document XUL document that contains the Loop panel > + * @type {XULElement} > + */ > +const getLoopBrowser = function(document) { Please make these underscore prefixed helper method on the UITour object to align with the others. I also try to keep as much non-tour code out of UITour.jsm so I would prefer if these were actually helpers in Loop code instead e.g. LoopUI. The advantage is that the code stays closer to the implementation so it's less likely to get overlooked in a refactoring. With that in mind, I also think a test for dealing with tabs would be good as I can see that breaking because it relies even more on the document structure.
Attachment #8560405 -
Flags: review?(MattN+bmo) → feedback+
Assignee | ||
Comment 17•9 years ago
|
||
Hi Matt, thanks for the excellent feedback! I think the changes you suggested are indeed for the better and allows for increased test coverage.
Attachment #8560405 -
Attachment is obsolete: true
Attachment #8561380 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8561380 -
Attachment is obsolete: true
Attachment #8561380 -
Flags: review?(MattN+bmo)
Attachment #8561381 -
Flags: review?(MattN+bmo)
Comment 19•9 years ago
|
||
Comment on attachment 8561381 [details] [diff] [review] Patch v3: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8561381 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/uitour/UITour.jsm @@ +138,5 @@ > }], > ["loop-newRoom", { > infoPanelPosition: "leftcenter topright", > query: (aDocument) => { > + let LoopUI = aDocument.contentWindow.LoopUI; aDocument.contentWindow? I think you want document.defaultView.LoopUI and I don't think aDocument.contentWindow exists. @@ +144,5 @@ > return null; > } > // Use the parentElement full-width container of the button so our arrow > // doesn't overlap the panel contents much. > + return loopUI.browser.contentDocument.querySelector(".new-room-button").parentElement; I guess you didn't test the patch since this should also be s/loopUI/LoopUI/ :) @@ +156,3 @@ > return null; > } > + return loopUI.browser.contentDocument.querySelector(".room-list"); Ditto about the case of "loopUI"
Attachment #8561381 -
Flags: review?(MattN+bmo) → review-
Assignee | ||
Comment 20•9 years ago
|
||
Brrr, indeed, I didn't test the patch :/
Assignee | ||
Comment 21•9 years ago
|
||
I added a test for the PanelTabChanged notification. If this doesn't work, I'll eat my hat. (I don't wear a hat).
Attachment #8561381 -
Attachment is obsolete: true
Attachment #8562039 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8562039 [details] [diff] [review] Patch v4: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8562039 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/uitour/test/browser_UITour_loop.js @@ +206,5 @@ > + gContentAPI.observe((event, params) => { > + is(event, "Loop:PanelTabChanged", "Check Loop:PanelTabChanged notification"); > + is(params, "contacts", "Check the tab name param"); > + > + // After nit: I removed this incomplete comment locally.
Comment 23•9 years ago
|
||
Comment on attachment 8562039 [details] [diff] [review] Patch v4: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Review of attachment 8562039 [details] [diff] [review]: ----------------------------------------------------------------- Thanks ::: browser/components/uitour/test/browser_UITour_loop.js @@ +198,5 @@ > + MozLoopServiceInternal.fxAOAuthProfile = fxASampleProfile; > + yield MozLoopServiceInternal.notifyStatusChanged("login"); > + > + // Show the Loop menu. > + yield new Promise(resolve => gContentAPI.showMenu("loop", () => resolve())); yield showMenuPromise("loop") (from head.js)
Attachment #8562039 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 24•9 years ago
|
||
Thanks! Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/b04123c901ac
Backed out in https://hg.mozilla.org/integration/fx-team/rev/f02908372053 for Marionette test bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=1957748&repo=fx-team
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 26•9 years ago
|
||
Sorry to have caused a backout, it was a simple fix: Re-landed as: https://hg.mozilla.org/integration/fx-team/rev/3fc96df801ff
Flags: needinfo?(mdeboer)
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3fc96df801ff
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8562039 [details] [diff] [review] Patch v4: notify UITour when the active tab changes and don't show the get started info panel when the rooms tab is not selected Approval Request Comment [Feature/regressing bug #]: UITour Fx36 [User impact if declined]: The info panel will be anchored to the edge of the Fx window when the contacts tab is selected. [Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass. [Risks and why]: minor. [String/UUID change made/needed]: n/a.
Attachment #8562039 -
Flags: approval-mozilla-beta?
Attachment #8562039 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8562039 -
Flags: approval-mozilla-beta?
Attachment #8562039 -
Flags: approval-mozilla-beta+
Attachment #8562039 -
Flags: approval-mozilla-aurora?
Attachment #8562039 -
Flags: approval-mozilla-aurora+
Comment 29•9 years ago
|
||
Needs rebasing for uplift.
Flags: needinfo?(mdeboer)
Keywords: branch-patch-needed
Assignee | ||
Comment 30•9 years ago
|
||
Pushed to release branches: https://hg.mozilla.org/releases/mozilla-aurora/rev/c5a75d4964d0, backed it out because I thought I missed something in https://hg.mozilla.org/releases/mozilla-aurora/rev/26753029dbf5, but these are the final SHA1's: https://hg.mozilla.org/releases/mozilla-aurora/rev/d2fad87c4e02 https://hg.mozilla.org/releases/mozilla-beta/rev/6c0ded9eb9aa
Updated•9 years ago
|
Keywords: branch-patch-needed
Assignee | ||
Comment 31•9 years ago
|
||
Corey, the UITour content will now receive a notification when the Loop panel tab changes: 'Loop:PanelTabChanged', with one param: {String} roomName. The values for `roomName` at this time may be 'rooms' or 'contacts'. When the 'rooms' tab is not selected to begin with when contents calls to show the info panels, they will be ignored and the panels won't be shown. I recommend to implement the following in the UITour content page: observe the 'Loop:PanelTabChanged' notification and hide the info panel for the 'Start Conversation' button and the room list when it switches to any tab other than 'rooms'.
Flags: needinfo?(cprice)
Assignee | ||
Comment 32•9 years ago
|
||
Sorry, it's Cory!
Comment 33•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #32) > Sorry, it's Cory! Ha, no problem :) I'll add this fix here bug1130198comment4
Flags: needinfo?(cprice)
Comment 34•9 years ago
|
||
While this event is working ok, as we discussed prior to implementing this solution - sometimes there is a delay between the tabs switching, and the web page being notified in time. On my machine, this can result in info-panels becoming incorrectly sized when not hidden in time (see attachment). This effects info panel sizing until the next time I restart Firefox.
Comment 35•9 years ago
|
||
Yeah, we knew this was likely to happen but it only affects certain configurations and it was the most straightforward solution. We'll see how many people complain.
Comment 36•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #31) > Corey, the UITour content will now receive a notification when the Loop > panel tab changes: 'Loop:PanelTabChanged', with one param: {String} > roomName. The values for `roomName` at this time may be 'rooms' or > 'contacts'. I see some activity on this bug, just want to add a quick note that the event is not quite working as described here. See manual[0]. There's no 'roomName' param. > 'Loop:PanelTabChanged' - User clicks on the Contacts or Room tab in the panel. > Event will return data = rooms or contacts depending on which tab the user clicked on. Noting because the tours are expecting the current behavior, so please don't change back :) [0] UITour readthedocs - https://bedrock.readthedocs.org/en/latest/uitour.html
Assignee | ||
Comment 37•9 years ago
|
||
(In reply to Cory Price [:ckprice] from comment #36) > (In reply to Mike de Boer [:mikedeboer] from comment #31) > > Corey, the UITour content will now receive a notification when the Loop > > panel tab changes: 'Loop:PanelTabChanged', with one param: {String} > > roomName. The values for `roomName` at this time may be 'rooms' or > > 'contacts'. > > I see some activity on this bug, just want to add a quick note that the > event is not quite working as described here. This is a new notification with the name 'Loop:PanelTabChanged' and the parameter passed in is a String, not an Object. The value of this string is the actual tab name. See also bug 1137141 for a current problem that this bug introduced.
You need to log in
before you can comment on or make changes to this bug.
Description
•