Closed Bug 1296322 Opened 3 years ago Closed 3 years ago

X button is vertically misaligned on the permission dropdown

Categories

(Firefox :: Site Identity, defect, P1)

51 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Iteration:
52.3 - Nov 14
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: pauly, Assigned: Paolo)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(2 files, 1 obsolete file)

[Affected versions]:
- 51.0a1 (2016-08-18)

[Affected platforms]:
- Win 7

[Steps to reproduce]:
1. Open any page
2. Open the Control Center, expand it, then click on More Info/Permissions
3. Change some permissions from the default state (e.g. set microphone to 'Block')
4. Open the Control Center again

[Expected result]:
- Allow/Block aligned with the X button

[Actual result]:
- misalignment between Allow/Block and the X button

[Regression range]:
- caused by bug 1290174

[Additional notes]:
- Windows only
Whiteboard: [fxprivacy] [triage]
Just to make sure, you mean _vertically_ misaligned, correct?
Yes
Summary: X button is misaligned on the permission dropdown → X button is vertically misaligned on the permission dropdown
Assignee: nobody → jkt
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: -- → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Is there a better component for this than "General"?  It isn't "Private Browsing", is it?
Version: Trunk → 51 Branch
There is one now!
Component: General → Site Identity and Permission Panels
Jonathan - do you think you will get to this in 51?  It's a regression that we'd rather not get into Aurora.
[Tracking Requested - why for this release]: Small visual regression that we should fix.
Tracking 51+ for this visual regression.
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Iteration: 51.2 - Aug 29 → 51.3 - Sep 19
Comment on attachment 8792020 [details]
Bug 1296322 - control center icon margin adjustment for different line size in windows

Hey Panos,
This patch is the first hack I could come up with to solve the alignment, the line height is different in Windows of the labels however changing the margin breaks at least Linux so I placed it around an ifdef.
I was looking for why all the icons are not aligned correctly in windows, it's like as if there is center alignment for the icons and baseline for the text. Perhaps we could file a follow up to look into it further?

Also I wasn't sure who to flag for review as Paolo is away.
Thanks!
Attachment #8792020 - Flags: review?(past)
Comment on attachment 8792020 [details]
Bug 1296322 - control center icon margin adjustment for different line size in windows

https://reviewboard.mozilla.org/r/79286/#review77820

I believe we want to avoid preprocessing nowadays, but we could do this platform-specific change in the usual way with a stylesheet per platform that includes the shared one.

Since this is likely to be a css-only fix I think we have some time to get a proper fix and uplift it to 51 later, but ideally we should get something like this in this train. dao, does this simple fix seem OK to you?
Attachment #8792020 - Flags: review?(past)
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Flags: qe-verify?
Comment on attachment 8792020 [details]
Bug 1296322 - control center icon margin adjustment for different line size in windows

- Is the number you used correct for different Windows versions (that use different fonts)? Does it still have the desired effect when changing the system-wide text size?

- Why did you use margin-block-start and not just margin-top?

- From the screenshot it looks like the icon is top-aligned? It should be center-aligned, and then you probably don't have to set a margin at all.
Attachment #8792020 - Flags: review?(dao+bmo) → review-
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Jonathan, is this still moving?  Would be nice to get it before 51 goes to beta.
Flags: needinfo?(jkt)
Unfortunately I have been pulled onto other things and this has slipped through the cracks, :elan would someone on the FXPrivacy team be able to pick this up? It should only be a small patch. I can keep this on my queue if there is nobody else however it probably makes most sense to throw back into being unassigned.
Assignee: jkt → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jkt) → needinfo?(elancaster)
Flags: needinfo?(elancaster)
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Duplicate of this bug: 1309576
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Whiteboard: [fxprivacy][triage] → [fxprivacy]
There seems to be no perfect solution here, because the elements are already vertically centered and the misalignment is caused by the typography of the font. At least on Windows 7, there is more extra space reserved for accents over uppercase letters than on other platforms.

Even aligning to the baseline is not something that can help here. If we adjusted the X so its lower part matched the baseline, it would still look misaligned with some font sizes.

Now I'm simply trying a middle ground, adjusting the font box slightly in a way that seems to work on most platforms. I'm now running try builds with mozscreenshots:

./mach try -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots["Ubuntu","10.10","Windows XP","Windows 7","Windows 8"] -t none --setenv MOZSCREENSHOTS_SETS=ControlCenter

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e808952fbdff008aeea06d90e2e7183efa9f9873
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a756c1f31db672b56f9a540bc21495c12ea1131e
Comment on attachment 8806028 [details]
Bug 1296322 - X button is vertically misaligned on the permission dropdown.

https://reviewboard.mozilla.org/r/89592/#review88964

The screenshots with the solution from comment 15 actually look decent to me on all platforms, so asking for review:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=e808952fbdff008aeea06d90e2e7183efa9f9873&newProject=try&newRev=a756c1f31db672b56f9a540bc21495c12ea1131e
Attachment #8792020 - Attachment is obsolete: true
Review ping.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail) → needinfo?(dao+bmo)
Comment on attachment 8806028 [details]
Bug 1296322 - X button is vertically misaligned on the permission dropdown.

Please add comments explaining what the negative margins are supposed to achieve here.

Have you checked larger font sizes to see that the relative offset (em) is indeed better than an absolute one (px)?

Generally, I'd prefer backing out bug 1290174. Bug 1290174 comment 0 was misguided: we were using the default system font size here (like we do for tabs for instance), so the concern that this could be hard to read to some users is moot. If users find this hard to read, there will be endless other places where they'll find text equally hard to read, and they should adjust the DPI or font size settings on their system.
Flags: needinfo?(dao+bmo)
Attachment #8806028 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [:dao] from comment #19)
> Generally, I'd prefer backing out bug 1290174.

This would help with bug 1299101 too.
(In reply to Dão Gottwald [:dao] from comment #19)
> Please add comments explaining what the negative margins are supposed to
> achieve here.

Done, I've also moved the margin to its own rule so I don't have to repeat or cross-reference the lengthy comment.

> Have you checked larger font sizes to see that the relative offset (em) is
> indeed better than an absolute one (px)?

Yes, the em value works better at intermediate font sizes. If the font is really large then it doesn't matter.

> Generally, I'd prefer backing out bug 1290174. Bug 1290174 comment 0 was
> misguided: we were using the default system font size here (like we do for
> tabs for instance), so the concern that this could be hard to read to some
> users is moot. If users find this hard to read, there will be endless other
> places where they'll find text equally hard to read, and they should adjust
> the DPI or font size settings on their system.

I'll raise this again with the team, we'll discuss at our next planning meeting. Maybe Philipp has an opinion on whether we really need the larger font size.
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Comment on attachment 8806028 [details]
Bug 1296322 - X button is vertically misaligned on the permission dropdown.

Carrying over r+, but feel free to let me know if you think the rule should be structured differently. I'll land this later today.
Attachment #8806028 - Flags: review?(dao+bmo) → review+
(In reply to :Paolo Amadini from comment #22)
> > Have you checked larger font sizes to see that the relative offset (em) is
> > indeed better than an absolute one (px)?
> 
> Yes, the em value works better at intermediate font sizes. If the font is
> really large then it doesn't matter.

This doesn't make sense. If the font is really large, the em value will translate to more pixels, so there must be some difference compared to using a px value.
Flags: qe-verify? → qe-verify+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad24787de436
X button is vertically misaligned on the permission dropdown. r=dao
(In reply to Dão Gottwald [:dao] from comment #24)
> (In reply to :Paolo Amadini from comment #22)
> > > Have you checked larger font sizes to see that the relative offset (em) is
> > > indeed better than an absolute one (px)?
> > 
> > Yes, the em value works better at intermediate font sizes. If the font is
> > really large then it doesn't matter.
> 
> This doesn't make sense. If the font is really large, the em value will
> translate to more pixels, so there must be some difference compared to using
> a px value.

I didn't mean that there wasn't a difference. As you have noted, with very large font sizes there is clearly a higher difference in the absolute position relative to the margin of the line. However, given the big difference in height between the icon and the font, there is no solution that "looks right", so one or the other doesn't really matter.
We need your guidance on font size, thank you.
Flags: needinfo?(philipp)
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Iteration: 52.2 - Oct 17 → 52.3 - Nov 14
https://hg.mozilla.org/mozilla-central/rev/ad24787de436
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
QA Contact: paul.silaghi
Please request Aurora/Beta uplift on this when you get a chance.
Flags: needinfo?(paolo.mozmail)
Sorry, I'm not quite sure what I am asked for here. Could you clarify?
Or has this been solved at this point (seeing that there's a patch)?
Flags: needinfo?(philipp)
I have reproduced this bug with Nightly 51.0a1 (2016-08-18) on Windows 7 , 64 Bit !

This bug's fix is now verified with latest Nightly 
 
Build ID    : 20161116030212
User Agent  : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
[bugday-20161116]
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #30)
> Sorry, I'm not quite sure what I am asked for here. Could you clarify?

This:

(In reply to :Paolo Amadini from comment #22)
> > Generally, I'd prefer backing out bug 1290174. Bug 1290174 comment 0 was
> > misguided: we were using the default system font size here (like we do for
> > tabs for instance), so the concern that this could be hard to read to some
> > users is moot. If users find this hard to read, there will be endless other
> > places where they'll find text equally hard to read, and they should adjust
> > the DPI or font size settings on their system.
> 
> I'll raise this again with the team, we'll discuss at our next planning
> meeting. Maybe Philipp has an opinion on whether we really need the larger
> font size.
Flags: needinfo?(philipp)
Verified fixed FX 53.0a1 (2016-11-16) based on comment 31.
Status: RESOLVED → VERIFIED
Hi :Paolo,
Since this is a regression, could you please nominate this uplift to Beta51 and Aurora52?
Comment on attachment 8806028 [details]
Bug 1296322 - X button is vertically misaligned on the permission dropdown.

Approval Request Comment
[Feature/regressing bug #]: Bug 1290174
[User impact if declined]: Suboptimal text and icon alignment in Control Center
[Describe test coverage new/current, TreeHerder]: MozScreenshots
[Risks and why]: Low, CSS margin change only
[String/UUID change made/needed]: None
Flags: needinfo?(paolo.mozmail)
Attachment #8806028 - Flags: approval-mozilla-beta?
Attachment #8806028 - Flags: approval-mozilla-aurora?
Comment on attachment 8806028 [details]
Bug 1296322 - X button is vertically misaligned on the permission dropdown.

Fix a UI regression. Beta51+ and Aurora52+. Should be in 51 beta 2.
Attachment #8806028 - Flags: approval-mozilla-beta?
Attachment #8806028 - Flags: approval-mozilla-beta+
Attachment #8806028 - Flags: approval-mozilla-aurora?
Attachment #8806028 - Flags: approval-mozilla-aurora+
I reproduced the issue on 51.0b1 build2  (20161115182233), using Windows 7 x64. The issue is verified fixed on 51.0b3 build1 (20161124073320) and on latest Aurora 52.0a2 (2016-11-25).
Well, looks like I managed to be late enough so that my needinfo isn't needed anymore.
FWIW, the current state in Nightly looks good to me.
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.