Add a restyled zoom control to static hamburger menu

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox56 verified)

Details

(Whiteboard: [photon-structure], )

Attachments

(1 attachment)

Assignee

Description

2 years ago
There's not much in this widget that remains the same, compared to the current zoom widget in the menu panel:

* There's a static label shown at the start of the menu item.
* The middle section consists of a spinner form control, which shows the zoom level of the currently visible page (tab/ browser) - just like the indicator in the location bar.
** The design shows a dropdown control to adjust the zoom level, but this is impossible to implement inside a panel. Use a spinner control instead.
** The zoom level can be adjusted by entering a new value, followed by <ENTER> in the control, or by using the mouse on the spinner buttons.
* An 'Enter Full Screen' button is shown at the end of the menu item, which shows it's caption in a tooltip when hovered.

A separator is shown below this menu item.
Assignee

Updated

2 years ago
Blocks: 1354108

Comment 1

2 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #0)
> ** The design shows a dropdown control to adjust the zoom level, but this is
> impossible to implement inside a panel. Use a spinner control instead.

Is this really true? Based on bug 964944, I kind of thought it was possible... might need thorough testing and/or a further 'experiment' bug though!
Flags: needinfo?(mdeboer)

Updated

2 years ago
No longer depends on: 1354087

Updated

2 years ago
No longer blocks: 1354108
Assignee

Comment 2

2 years ago
OK, I don't know, TBH. I think it's a good idea to ask Bryan which control he'd prefer.

Bryan. for the zoom control widget inside the static menu panel/ hamburger menu, whatever, would you prefer a select dropdown or a spinner control (which has +/- buttons)?

If you prefer a select dropdown, what will the value of each option be?
Flags: needinfo?(mdeboer) → needinfo?(bbell)
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Updated

2 years ago
Depends on: 1359625
Bug to consider:
bug #1363679 -  Zooming with:
     +/- Zoom Controls buttons in hamburger menu,
     Zoom In/Out buttons in Menu Bar in View menu in Zoom submenu,
     Ctrl + +/- keyboard shortcuts;
- are inconsistent with zooming with Mouse Scroll,
so when users don't have mouse with scroll, they loose massive amount of zoom control.
Assignee

Updated

2 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1

Comment 6

2 years ago
mozreview-review
Comment on attachment 8873075 [details]
Bug 1354105 - Add zoom controls - zoom in, zoom reset, zoom out and fullscreen - buttons to the Photon app menu.

https://reviewboard.mozilla.org/r/144526/#review148410

I think this misses the full screen button? Sorry if it's not obvious - but that's part of the same <toolbaritem> in the spec and I didn't file a separate bug. It *is* also mentioned in comment #0... :-)

If you want to push it to a followup, that's also fine, up to you.

From a quick glance I'm a little confused about the styling changes - based on https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/230516723 the distance between the edit and zoom buttons is different, in order for them to line up vertically. It looks like your CSS changes just reduce the spacing for both sets of buttons?
Attachment #8873075 - Flags: review?(gijskruitbosch+bugs)
Assignee

Comment 7

2 years ago
(In reply to :Gijs from comment #6)
> I think this misses the full screen button? Sorry if it's not obvious - but
> that's part of the same <toolbaritem> in the spec and I didn't file a
> separate bug. It *is* also mentioned in comment #0... :-)

Argh! Forgot about that one :/ I'll add it in the next push.

> From a quick glance I'm a little confused about the styling changes - based
> on https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/230516723 the
> distance between the edit and zoom buttons is different, in order for them
> to line up vertically. It looks like your CSS changes just reduce the
> spacing for both sets of buttons?

Yeah, that's a bad on my part. I didn't compare both items good enough.
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8873075 [details]
Bug 1354105 - Add zoom controls - zoom in, zoom reset, zoom out and fullscreen - buttons to the Photon app menu.

https://reviewboard.mozilla.org/r/144526/#review148762

Most of my comments about that ruddy vertical separator. ;-)

But this looks good, and I trust you to deal with that, so r=me after the below issues are fixed. Don't need to see it again unless you think I do. Thanks! :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:1345
(Diff revision 2)
> +photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons >
> +  .subviewbutton-iconic > .toolbarbutton-text,

Is the `.PanelUI-subView` selector really required here?

::: browser/themes/shared/customizableui/panelUI.inc.css:1347
(Diff revision 2)
> +photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons >
> +  .subviewbutton:not(.subviewbutton-iconic) > .toolbarbutton-icon {

I wonder if this second selector should just be #appMenu-zoomReset-button > .toolbarbutton-icon ... there aren't any other items, are there? We don't want it to apply to buttons outside the combined-buttons set, even if they don't have icons, so I think that would be OK, plus it makes the selector more efficient and easier to read. :-)

::: browser/themes/shared/customizableui/panelUI.inc.css:1361
(Diff revision 2)
> +  border: 1px solid #e1e1e1;
> +  border-radius: 10000px;
> +  padding: 1px 8px;
> +}
> +
> +photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons > 

Nit: trailing space

::: browser/themes/shared/customizableui/panelUI.inc.css:1496
(Diff revision 2)
> +.PanelUI-subView toolbarseparator[orient="vertical"] {
> +  height: 24px;

This surprises me, for 2 reasons. First, it's not in a photon block, and the selector looks like it'd apply to non-photon stuff, which is a bit scary. Second, I already added a similar set of code for the sync separator. The selector looks like this:

#appMenu-fxa-container[fxastatus="signedin"] > toolbarseparator 

and it lives in this file. Judging by the code comment, I guess you might need to override the background-color for this to look good on OS X? Or perhaps I just did a shoddy job of the other separator and I could have done it more concisely, somehow?

Regardless, can we unify these and make them the same color/size/mechanism (background vs. border)? (Funny thing, we also seem to use a different colour...)

It looks like the margins might need to be separate, which is fine - but we should try to share the rest of the styles, I think.
Attachment #8873075 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 11

2 years ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb9964c00980
Add zoom controls - zoom in, zoom reset, zoom out and fullscreen - buttons to the Photon app menu. r=Gijs

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb9964c00980
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 13

2 years ago
On Windows: When I hit the full screen toggle control via the hamburger menu, the photon hamburger menu stays.
On Ubuntu/Mac: When I hit the full screen toggle control via the hamburger menu, the photon hamburger menu disappears.

Is this expected functionality for the operating systems? If not, which scenario is the intended one? Thanks!
Flags: needinfo?(mdeboer)
Assignee

Comment 14

2 years ago
Hi Grover, thanks for checking!

The intended behavior is that the menu should disappear on all operating systems. If that doesn't happen on Windows, then that's odd and a bug that'd block this one. For tracking purposes I think it'd be best to have a new bug for it and not re-opening this one.
Flags: needinfo?(mdeboer)

Updated

2 years ago
Depends on: 1371311

Comment 15

2 years ago
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Blocks: 1387512
Depends on: 1418561
You need to log in before you can comment on or make changes to this bug.