Closed Bug 1402929 Opened 4 years ago Closed 4 years ago

Clearer labeling of overflow panel and its dropzone targets in customize mode

Categories

(Firefox :: Toolbars and Customization, defect, P1)

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: abenson, Assigned: Gijs)

References

()

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(4 files)

Have re-organized the design of the Overflow Panel and detailed some drop targets to help clarify how the panel can be used. 

https://mozilla.invisionapp.com/share/TJDNCM54F#/screens/255104764_Customize_-_Overflow_Panel_Updates
Whiteboard: [photon-structure] → [photon-structure][triage]
Flags: qe-verify+
Priority: -- → P4
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Priority: P4 → --
QA Contact: gwimberly
Whiteboard: [reserve-photon-structure] → [photon-structure][triage]
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Priority: -- → P3
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Per discussion with Aaron, no grey bar thingy, but I made the other changes suggested. I put the header in its own commit because it's not upliftable. We can uplift the other change.
osx build for testing: https://queue.taskcluster.net/v1/task/Bpr2ujM0QL6OXXLAnMt8-A/runs/0/artifacts/public/build/target.dmg
linux64: https://queue.taskcluster.net/v1/task/PF-BgF_ORSOe-AnqVgsaWw/runs/0/artifacts/public/build/target.tar.bz2
win64: https://queue.taskcluster.net/v1/task/U_gLxqQnSueD8mW9s4MpXQ/runs/0/artifacts/public/build/target.zip

Aaron, can you check this matches your expectations? The items in the overflow panel currently have 12px padding everywhere, so I kept this for the header as well so they line up. I don't really want to risk changing everything to 10px now (for 57) given that the styling is shared with the hamburger panel etc. and it's been tricky to get items with & without icons to line up on all platforms in all cases. I also used relative font-sizing to get the header and description fonts. I think otherwise this matches the spec.
Flags: needinfo?(abenson)
Comment on attachment 8913879 [details]
Bug 1402929 - try to make overflow panel in customize mode easier to understand,

https://reviewboard.mozilla.org/r/185276/#review190516

This is awesome! Didn't expect this much of an improvement in usability... I have two non-blocking (no pun intended) comments.

::: commit-message-cd9c8:5
(Diff revision 1)
> +Bug 1402929 - try to make overflow panel in customize mode easier to understand, r?mikedeboer
> +
> +This moves the explanatory text from a pseudoelement to a real element,
> +and keeps only the image in a pseudoelement. It then uses a transitioned
> +opacity to change to fade that image in and out.

nit: to change or to fade?

::: browser/themes/shared/customizableui/customizeMode.inc.css:441
(Diff revision 1)
>    border-radius: var(--arrowpanel-border-radius);
>  %else
>    border: 1px solid var(--arrowpanel-border-color);
>    box-shadow: 0 0 4px hsla(0,0%,0%,.2);
>  %endif
> +  max-width: 29em;

You don't have access to @wideMenuPanelWidth@ here?
Attachment #8913879 - Flags: review?(mdeboer) → review+
Comment on attachment 8913880 [details]
Bug 1402929 - add header to overflow panel in customize mode,

https://reviewboard.mozilla.org/r/185278/#review190520

Nice to have this separated out. r=me.
Attachment #8913880 - Flags: review?(mdeboer) → review+
Gijs .. this looks fantastic!!!
Flags: needinfo?(abenson)
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> ::: browser/themes/shared/customizableui/customizeMode.inc.css:441
> (Diff revision 1)
> >    border-radius: var(--arrowpanel-border-radius);
> >  %else
> >    border: 1px solid var(--arrowpanel-border-color);
> >    box-shadow: 0 0 4px hsla(0,0%,0%,.2);
> >  %endif
> > +  max-width: 29em;
> 
> You don't have access to @wideMenuPanelWidth@ here?

Sadly, no, the define is in panelUI.inc.css (included in platform-specific panelUI.css), and this is in customizeMode.inc.css, which gets included in platform-specific browser.css . I'll file a followup to make this more sane. Could probably be a CSS variable.
hg error in cmd: hg rebase -s 6fcf0a6fb13c -d 07e7579a07be: abort: can't rebase public changeset 6fcf0a6fb13c
(see 'hg help phases' for details)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2c3125cbc8f5
try to make overflow panel in customize mode easier to understand, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/023950f209ee
add header to overflow panel in customize mode, r=mikedeboer
Comment on attachment 8913879 [details]
Bug 1402929 - try to make overflow panel in customize mode easier to understand,

Approval Request Comment
[Feature/Bug causing the regression]: Using overflow panel as the customizable panel in customize mode as part of photon.
[User impact if declined]: confusing UX in customize mode when putting things in the panel. This used to be more obvious because the hamburger panel wasn't empty by default.
[Is this code covered by automated tests?]: the UX isn't, the general functionality of dragging items to the panel is
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: verification the new UI/UX is in place would be good. No specific STR beyond opening customize mode and checking the experience of the drag/drop to/from the panel. Note that we explicitly decided not to implement the grey placement bar in the panel to indicate where an item would be dropped.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky
[Why is the change risky/not risky?]: it's a relatively small, frontend-only patch that changes the image positioning etc. in the overflow panel, as well as making it fade in/out when dragging items. There's pretty good automated test coverage of the general drag/drop interaction in customize mode, and not that many ways this code could interfere with the functionality - plus we still have quite a few weeks of beta left.
[String changes made/needed]: not in this patch

Note: APPROVAL REQUEST ONLY FOR THIS CSET, NOT THE SECOND ONE
The second patch includes strings and won't make 57. In 57 we'll ship the new look without the "Overflow Panel" header, which will appear in 58. Still, the patch will be an improvement.
Attachment #8913879 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/2c3125cbc8f5
https://hg.mozilla.org/mozilla-central/rev/023950f209ee
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Introducing customizeMode.overflowList.description wasn't really needed, since the content is the same as customizeMode.emptyOverflowList.description

Having said that, is the name "Overflow Panel" now? Because it's been called "Overflow menu" so far in the UI
https://transvision.mozfr.org/?recherche=Overflow+menu&repo=central&sourcelocale=en-US&locale=en-US&search_type=strings_entities
Flags: needinfo?(abenson)
(In reply to Francesco Lodolo [:flod] from comment #17)
> Introducing customizeMode.overflowList.description wasn't really needed,
> since the content is the same as customizeMode.emptyOverflowList.description

But we now use it when the list isn't empty, so the name was no longer correct, and on balance I decided that causing confusion for translators "forever" from now was worse than the 1-time pain of changing the label. Anyway, l20n is going to fix all of this pain, right...?

> Having said that, is the name "Overflow Panel" now? Because it's been called
> "Overflow menu" so far in the UI
> https://transvision.mozfr.org/
> ?recherche=Overflow+menu&repo=central&sourcelocale=en-US&locale=en-
> US&search_type=strings_entities

--> Aaron?
(In reply to :Gijs from comment #18)
> (In reply to Francesco Lodolo [:flod] from comment #17)
> > Introducing customizeMode.overflowList.description wasn't really needed,
> > since the content is the same as customizeMode.emptyOverflowList.description
> 
> But we now use it when the list isn't empty, so the name was no longer
> correct, and on balance I decided that causing confusion for translators
> "forever" from now was worse than the 1-time pain of changing the label.
> Anyway, l20n is going to fix all of this pain, right...?

Ehm, not really ;-) A string ID is a string ID, so this case would remain the same.

We're trying to find a way to solve "I want to change the string content, invalidating translations but without touching code and using a new ID", but that's hard and still on the planning table.
Depends on: 1405643
Comment on attachment 8913879 [details]
Bug 1402929 - try to make overflow panel in customize mode easier to understand,

We should probably hold off uplifting this until bug 1405643 is fixed.
Attachment #8913879 - Flags: approval-mozilla-beta?
Spoke to Gijs about the panel/menu issue. Menu is the more common term so we should change the title in the overflow "menu" to read Overflow Menu.
Flags: needinfo?(abenson)
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171004100049

This issue has been verified on latest Firefox Nightly Build ID: 20171004100049 on Windows 8.1 x64, Mac OS 10.11 and Ubuntu 14.04. On Mac and Ubuntu all is ok.
 
On Windows the "Overflow Panel" Headline is not bolded. This seems to be an issue compared with the rest of OSs where the Headline is bolded. There should be consistency between them.

Is this expected?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Vlad Bacia-Mociran [:VladB] from comment #22)
> On Windows the "Overflow Panel" Headline is not bolded. This seems to be an
> issue compared with the rest of OSs where the Headline is bolded. There
> should be consistency between them.
> 
> Is this expected?

I talked with Aaron and we think this is OK as-is. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
Ni me to put a patch together that's upliftable with the relevant 2 csets (1 from here, 1 from bug 1405643, no strings).
Flags: needinfo?(gijskruitbosch+bugs)
Vlad, were there any other issues you found besides the bold state of the title, or is this verified on 58?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(vbacia.work)
Attached patch Patches for betaSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Using overflow panel as the customizable panel in customize mode as part of photon.
[User impact if declined]: confusing UX in customize mode when putting things in the panel. This used to be more obvious because the hamburger panel wasn't empty by default.
[Is this code covered by automated tests?]: the UX isn't, the general functionality of dragging items to the panel is.
[Has the fix been verified in Nightly?]: I believe so, see comment 25 (and any replies, maybe, by the time you read this).
[Needs manual test from QE? If yes, steps to reproduce]: No specific STR beyond opening customize mode and checking the experience of the drag/drop to/from the panel, both when it's empty and when it's not empty. Note that we explicitly decided not to implement the grey placement bar in the panel to indicate where an item would be dropped.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky
[Why is the change risky/not risky?]: it's a relatively small, frontend-only patch that changes the image positioning etc. in the overflow panel, as well as making it fade in/out when dragging items. There's pretty good automated test coverage of the general drag/drop interaction in customize mode, and not that many ways this code could interfere with the functionality - plus we still have quite a few weeks of beta left.
[String changes made/needed]: not in this patch

Note: this combines the first cset from this bug and the first cset of bug 1405643. It doesn't have the second csets from those bugs because those contain strings.
Attachment #8915765 - Flags: review+
Attachment #8915765 - Flags: approval-mozilla-beta?
(In reply to :Gijs from comment #26)
> [Has the fix been verified in Nightly?]: I believe so, see comment 25 (and
> any replies, maybe, by the time you read this).

Err, and comment 22. :-)
(In reply to :Gijs (not here until monday) from comment #25)
> Vlad, were there any other issues you found besides the bold state of the
> title, or is this verified on 58?

I didn't encountered any other issues. The changes have been verified on on Nightly v58.0a1.
Flags: needinfo?(vbacia.work)
Comment on attachment 8915765 [details] [diff] [review]
Patches for beta

Photon related, Beta57+
Attachment #8915765 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No issues were encountered while testing this (by following the guidelines provided in comment 26) on Firefox 57.0b9 (BuildId:20171016185129) using Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1567658
You need to log in before you can comment on or make changes to this bug.