Loop: info panel position breaks when "Contacts" tab is activated in Hello panel

RESOLVED FIXED in Firefox 36

Status

()

Firefox
Tours
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cstipkovic, Assigned: mikedeboer)

Tracking

unspecified
Firefox 38
Points:
3
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

(Whiteboard: [UITour:P2])

Attachments

(4 attachments, 4 obsolete attachments)

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
(Reporter)

Description

2 years ago
Created attachment 8548475 [details]
Welcome box broken when contacts tab activated

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

2 years ago
Created attachment 8548477 [details]
The default behavior when Online tab is activated
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@gmail.com, reuben.bmo@gmail.com
Blocks: 1080942
(Reporter)

Comment 3

2 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

2 years ago
Duplicate of this bug: 1121401
Blocks: 1118874
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)
(Assignee)

Comment 8

2 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

2 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

2 years ago
Created 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
Attachment #8558507 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

2 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 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.
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
(Assignee)

Comment 14

2 years ago
Created 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

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

2 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 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

2 years ago
Created attachment 8561380 [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

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

2 years ago
Created 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
Attachment #8561380 - Attachment is obsolete: true
Attachment #8561380 - Flags: review?(MattN+bmo)
Attachment #8561381 - 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-
(Assignee)

Comment 20

2 years ago
Brrr, indeed, I didn't test the patch :/
(Assignee)

Comment 21

2 years ago
Created 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

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

2 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 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

2 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

2 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)
https://hg.mozilla.org/mozilla-central/rev/3fc96df801ff
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox38: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
(Assignee)

Comment 28

2 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?
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)
Keywords: branch-patch-needed
(Assignee)

Comment 30

2 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
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Flags: needinfo?(mdeboer)
Keywords: branch-patch-needed
(Assignee)

Comment 31

2 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

2 years ago
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)
Created attachment 8566539 [details]
door-hanger-odd-size-size.jpg

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.
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 37

2 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.