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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: abenson, Assigned: Gijs)

Tracking

57 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure], )

Attachments

(4 attachments)

Reporter

Description

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

Updated

2 years ago
Whiteboard: [photon-structure] → [photon-structure][triage]
Flags: qe-verify+
Priority: -- → P4
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Assignee

Updated

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

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 3

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

Comment 4

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

2 years ago
mozreview-review
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 6

2 years ago
mozreview-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+
Reporter

Comment 7

2 years ago
Gijs .. this looks fantastic!!!
Flags: needinfo?(abenson)
Assignee

Comment 8

2 years ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
hg error in cmd: hg rebase -s 6fcf0a6fb13c -d 07e7579a07be: abort: can't rebase public changeset 6fcf0a6fb13c
(see 'hg help phases' for details)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

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

Comment 15

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

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c3125cbc8f5
https://hg.mozilla.org/mozilla-central/rev/023950f209ee
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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)
Assignee

Comment 18

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

Updated

2 years ago
Depends on: 1405643
Assignee

Comment 20

2 years ago
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?
Reporter

Comment 21

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

Comment 23

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

Comment 24

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

Comment 25

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

Comment 26

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

Comment 27

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

Updated

2 years ago
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+
You need to log in before you can comment on or make changes to this bug.