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)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

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+
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.
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
Note that this can only happen if the user signed in with FxA otherwise the contact tab doesn't show.
Mentor: felipc, reuben.bmo
[:MattN], yes, you're right, I forgot to mention that I'm using the FxA.

Thanks [:ckprice] and [:MattN] for improve this bug description :)
Depends on: 1109868
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]
(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.
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)
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)
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.
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 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+
(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 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.
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)
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 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+
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)
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-
Brrr, indeed, I didn't test the patch :/
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)
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 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+
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)
https://hg.mozilla.org/mozilla-central/rev/3fc96df801ff
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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?
Attachment #8562039 - Flags: approval-mozilla-beta?
Attachment #8562039 - Flags: approval-mozilla-beta+
Attachment #8562039 - Flags: approval-mozilla-aurora?
Attachment #8562039 - Flags: approval-mozilla-aurora+
Needs rebasing for uplift.
Flags: needinfo?(mdeboer)
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)
Sorry, it's Cory!
(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)
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.
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.
Depends on: 1137141
(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
(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.