Add a restyled zoom control to static hamburger menu

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
5 months ago
20 days 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], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months 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

5 months ago
Blocks: 1354108

Comment 1

5 months 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

5 months ago
No longer depends on: 1354087

Updated

5 months ago
No longer blocks: 1354108
(Assignee)

Comment 2

5 months 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)

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Updated

4 months 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.

Comment 4

3 months ago
Mock-up: https://mozilla.invisionapp.com/share/H3BIW4C9U#/230516723_Application_Menu
Flags: needinfo?(bbell)
(Assignee)

Updated

3 months ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 6

3 months 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

3 months 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

3 months 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

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb9964c00980
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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

3 months 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)
Depends on: 1371311
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox56: --- → verified
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.