Consolidate <radio> bindings across platforms

RESOLVED FIXED in Firefox 58

Status

()

Toolkit
Themes
P2
normal
RESOLVED FIXED
28 days ago
11 days ago

People

(Reporter: bgrins, Assigned: dao)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [xbl-flatten-inheritance])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

28 days ago
From https://bugzilla.mozilla.org/show_bug.cgi?id=1410540#c6:
> we should get rid of the platform-specific bindings, which should simplify
> this patch. I did that for <checkbox/> in bug 1345432

For Windows/Linux, we have https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml which defines the "radio-with-spacing" and "radio" bindings.
- Linux: <radio> is bound to "radio-with-spacing": https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#17
- Windows: <radio> is bound to "radio": https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#17

For OSX we use the standard "radio" binding defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/radio.xml#431.  We also use the standard radio binding cross platform in various places, like subviews in the panelUI: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1917.

We should at least remove "radio-with-spacing" as in Bug 1345432, but also look into getting rid of the overridden "radio" binding for Windows and make all platforms use the binding defined in radio.xml.
(Assignee)

Updated

27 days ago
Priority: -- → P2
(Reporter)

Updated

26 days ago
Blocks: 1412369
(Assignee)

Updated

20 days ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Blocks: 1341048
See Also: → bug 1347111
(Assignee)

Updated

20 days ago
Attachment #8924528 - Flags: review?(bgrinstead)
(Reporter)

Comment 2

20 days ago
Mozscreenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=40a14ca1cf04499f398e4cb8ba359b39eae4e216&newProject=try&newRev=6011efa03c457728a9d9b2caf10d0ee8eebc62f0.

- Looks like some slight spacing changes on in content prefs that may not be a big deal: https://screenshots.mattn.ca/comparisons/mozilla-central/40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/preferences_01_prefsGeneral.png 
- And some checkbox spacing changes, which I assume are unintended: https://screenshots.mattn.ca/comparisons/mozilla-central/40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/6011efa03c457728a9d9b2caf10d0ee8eebc62f0/windows10-64/permissionPrompts_01_noLWT_shareDevices.png.
(Reporter)

Comment 3

20 days ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Mozscreenshots:
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=40a14ca1cf04499f398e4cb8ba359b39eae4e216&newProject=try&newRev
> =6011efa03c457728a9d9b2caf10d0ee8eebc62f0.

There are some false positives there having to do with the Nightly icon i.e. https://screenshots.mattn.ca/comparisons/mozilla-central/40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/controlCenter_001_noLWT_about.png. Matt, is that something that is expected?  The base is an m-c push and the new is a try push.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 4

20 days ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> - And some checkbox spacing changes, which I assume are unintended:
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/windows10-64/
> permissionPrompts_01_noLWT_shareDevices.png.

It's expected ... there was extra space due to this bogus rule:

.checkbox-icon {
  margin-inline-end: 2px;
}
(Reporter)

Comment 5

20 days ago
(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > - And some checkbox spacing changes, which I assume are unintended:
> > https://screenshots.mattn.ca/comparisons/mozilla-central/
> > 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> > 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/windows10-64/
> > permissionPrompts_01_noLWT_shareDevices.png.
> 
> It's expected ... there was extra space due to this bogus rule:
> 
> .checkbox-icon {
>   margin-inline-end: 2px;
> }

OK sounds fine, I did see that change but wasn't sure why it was there
Comment hidden (mozreview-request)
(Assignee)

Comment 7

20 days ago
(In reply to Brian Grinstead [:bgrins] from comment #2)
> - Looks like some slight spacing changes on in content prefs that may not be
> a big deal:
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/preferences_01_prefsGeneral.
> png 

fixed with the latest patch
(Reporter)

Comment 8

20 days ago
OK, I will re-run mozscreenshots on the latest version using a try push for the base so we don't get the logo false positives
(Reporter)

Comment 9

20 days ago
New mozscreenshot:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=6bc0b22a473cae8f91e5d6d5ac9a828362e8351d&newProject=try&newRev=0b6a324628ed8e5206a19a2a568e0a13db75b772

Besides the known windows permission prompt checkbox move I see:

- Very slight radio move on OSX in prefs: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/preferences_01_prefsGeneral.png

A couple of things that look like issues with mozscreenshots:

- Text centered on Linux tab on the base rev: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/linux64/controlCenter_017_noLWT_httpPassword.png
- Panel shadow and background color changes on OSX: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/controlCenter_026_darkLWT_https.png
- URL text rendering difference on OSX: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/permissionPrompts_33_compactLight_shareVideoAndMicrophone.png
- Border radius difference on windows on about:home https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/windows10-64/primaryUI_001_tabsInTitlebar_fiveTabs_maximized_onlyNavBar_noLWT.png
- Slight desktop background difference (not clipping fully) on Windows: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/windows10-64/primaryUI_161_tabsOutsideTitlebar_twoPinnedWithOverflow_normal_onlyNavBar_noLWT.png
(Reporter)

Updated

20 days ago
Whiteboard: [xbl-flatten-inheritance]
(Assignee)

Comment 10

20 days ago
(In reply to Brian Grinstead [:bgrins] from comment #9)
> New mozscreenshot:
> 
> https://screenshots.mattn.ca/compare/
> ?oldProject=try&oldRev=6bc0b22a473cae8f91e5d6d5ac9a828362e8351d&newProject=tr
> y&newRev=0b6a324628ed8e5206a19a2a568e0a13db75b772
> 
> Besides the known windows permission prompt checkbox move I see:
> 
> - Very slight radio move on OSX in prefs:
> https://screenshots.mattn.ca/comparisons/try/
> 6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/
> 0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/
> preferences_01_prefsGeneral.png

That's good, it means that Mac was suffering from the bug you reported for Linux in comment 2, and my patch fixes that.
Whiteboard: [xbl-flatten-inheritance]
(Assignee)

Updated

20 days ago
Whiteboard: [xbl-flatten-inheritance]
(Reporter)

Comment 11

20 days ago
mozreview-review
Comment on attachment 8924528 [details]
Bug 1411640 - Consolidate <radio> bindings and styling across platforms.

https://reviewboard.mozilla.org/r/195790/#review201212

This is _much_ simpler, thanks! I don't see any visual regressions and the changes look reasonable to me
Attachment #8924528 - Flags: review?(bgrinstead) → review+

Comment 12

19 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e49de4bbc6f
Consolidate <radio> bindings and styling across platforms. r=bgrins
Backed out for failing browser-chrome's browser/base/content/test/static/browser_all_files_referenced.js, at least on Windows:

https://hg.mozilla.org/integration/autoland/rev/4dc4eae45e0a8c6562692a8509b512a8b6a088cb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4e49de4bbc6f0647dd82b752e52961106e7d4879&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141936490&repo=autoland

119 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0
120 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://global/skin/globalBindings.xml -
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 14

19 days ago
Huh - I don't see any reference to "toolbarpaletteitem-spacer" other than the definition in globalBindings.xml, so it looks like that file could be removed!

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml?q=toolbarpaletteitem-spacer&redirect_type=single#46
Comment hidden (mozreview-request)
(Assignee)

Updated

19 days ago
Flags: needinfo?(dao+bmo)

Comment 16

19 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1215f43a62a
Consolidate <radio> bindings and styling across platforms. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/e1215f43a62a
Status: ASSIGNED → RESOLVED
Last Resolved: 18 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Mozscreenshots:
> > https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> > central&oldRev=40a14ca1cf04499f398e4cb8ba359b39eae4e216&newProject=try&newRev
> > =6011efa03c457728a9d9b2caf10d0ee8eebc62f0.
> 
> There are some false positives there having to do with the Nightly icon i.e.
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/
> controlCenter_001_noLWT_about.png. Matt, is that something that is expected?
> The base is an m-c push and the new is a try push.

This is a problem specific to artifact try pushes and I've filed bug 1416538 to address it. In the meantime use the `--no-artifact` option to `mach try fuzzy` if you're going to compare with m-c. I've added a warning to https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots#Try_pushes
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.