Closed Bug 1396085 Opened 7 years ago Closed 7 years ago

Photon menu killed the Sync calls to action

Categories

(Firefox :: Sync, defect)

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: rfeeley, Assigned: markh)

Details

Attachments

(3 files)

STEPS TO REPRODUCE
Sign in to Sync on only one device
Open Synced Tabs panel

EXPECTED RESULTS
Links should be there link in the sidebar

ACTUAL RESULTS
They were removed, I believe during Photon migration
I can look into this and provide some more UX specs... NI'ing myself.

Ryan, can you detail which links need to be there and define any special states for the UI? Thanks!
Flags: needinfo?(rfeeley)
Flags: needinfo?(abenson)
Exactly the copy that is there in the sidebar. It used to be there, but it vanished in the new menu.
Flags: needinfo?(rfeeley)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Comment on attachment 8904417 [details]
Bug 1396085 - ensure the sync widget is always initialized and animate the sync spinner.

https://reviewboard.mozilla.org/r/176228/#review181162

::: browser/components/customizableui/CustomizableWidgets.jsm:259
(Diff revision 1)
>        DECKINDEX_NOCLIENTS: 3,
>      },
>      TABS_PER_PAGE: 25,
>      NEXT_PAGE_MIN_TABS: 5, // Minimum number of tabs displayed when we click "Show All"
>      onCreated(aNode) {
> +      this._initialize(aNode);

This is a little suspect - I'm not sure what the semantics are for onCreated, but there may be a better way to perform a one-off initialization for the widget?
Comment on attachment 8904417 [details]
Bug 1396085 - ensure the sync widget is always initialized and animate the sync spinner.

https://reviewboard.mozilla.org/r/176228/#review181318

r=me if you unify the selector for the animation.

::: browser/components/customizableui/CustomizableWidgets.jsm:259
(Diff revision 1)
>        DECKINDEX_NOCLIENTS: 3,
>      },
>      TABS_PER_PAGE: 25,
>      NEXT_PAGE_MIN_TABS: 5, // Minimum number of tabs displayed when we click "Show All"
>      onCreated(aNode) {
> +      this._initialize(aNode);

I don't think there is, so this is fine for now.

::: browser/themes/shared/menupanel.inc.css:90
(Diff revision 1)
> +  animation: syncRotate 0.8s linear infinite;
> +  fill: #0a84ff;

Can you just add this selector to https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#809 ? We now technically have 2 identically-named keyframes, which doesn't seem good.
Attachment #8904417 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/fc8ba9723756
ensure the sync widget is always initialized and animate the sync spinner. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/fc8ba9723756
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: needinfo?(abenson)
Attached image off.gif
The link is there now, but the menu height has gone wild.
Assignee: markh → rfeeley
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Why did you assign this to yourself?

Are you actually sure this bug regressed the panel height thing? Generally, IME it's more useful to file a separate followup bug unless it's really clear that the patch in the bug just did nothing good and/or broke the entire world, which I don't think is the case here.
Flags: needinfo?(rfeeley)
I wonder, for instance, if bug 1374315 caused the situation in attachment 8905161 [details]...
I think Ryan just misunderstood how we use Bugzilla - I opened bug 1397586 for this new issue.
Assignee: rfeeley → markh
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(rfeeley)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.