Remove windows-{xp,vista} values for the -moz-os-version CSS property

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: njn, Assigned: miketaylr)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Places that need changing include the following:

- Update osVersionStrings in layout/style/nsMediaFeatures.cpp

- Update OperatingSystemVersion in widget/LookAndFeel.h

- Update windows_versions in layout/style/test/chrome/bug418986-2.js

- Remove uses in layout/style/test/test_media_queries.html and browser/components/extensions/extension-win-panel.css
(Reporter)

Comment 1

a year ago
Also remove a use in browser/themes/shared/tab-selected.svg.
Just to confirm: this won't cause existing pages that use xp/vista media queries to go crazy (error out catastrophically, etc), right?
(Reporter)

Comment 4

a year ago
(In reply to David Major [:dmajor] from comment #3)
> Just to confirm: this won't cause existing pages that use xp/vista media
> queries to go crazy (error out catastrophically, etc), right?

Good question! I don't know. I'm not planning to work on this myself. Perhaps someone who know more about CSS can answer. jimm, do you know?
Flags: needinfo?(jmathies)
Hmm, we probably should keep the queries, and just make sure they alwasy return false. This is more a question for web compat, ni'ing mtaylor.
Flags: needinfo?(jmathies) → needinfo?(miket)
Media features with invalid values are just ignored, which is the same as always returning false. So this change won't break any sites. Unless of course, if your site was relying on them for some (sad) reason.

(I guess we add a "Found invalid value for media feature." message in the console as well).
Flags: needinfo?(miket)
No longer blocks: 1130266
Depends on: 1130266
Blocks: 1130266
No longer depends on: 1130266

Updated

7 months ago
Priority: -- → P3
(Looks like this was partially done in Bug 1325503)
Assignee: nobody → miket
Comment hidden (mozreview-request)
(Reporter)

Comment 9

7 months ago
mozreview-review
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review163942

This seems reasonable to me but I don't really know anything about these properties, so I don't think I'm qualified to review this. Sorry!
Attachment #8887650 - Flags: review?(n.nethercote)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
No worries! Lemme ping bwinton since he last reviewed code for extension-win-panel.css, and I'll ping a layout peer.

Comment 13

7 months ago
mozreview-review
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review164264

::: commit-message-1f894:1
(Diff revision 2)
> +Bug 1330146. Remoev windows-xp and windows-vista values from extension-win-panel.css. r=bwinton

drive-by -- typo:  "Remoev"

Comment 14

7 months ago
mozreview-review
Comment on attachment 8887981 [details]
Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests.

https://reviewboard.mozilla.org/r/158850/#review164266

r=me, perhaps with a bit more detail in the extended commit message as noted below:

::: commit-message-41349:1
(Diff revision 1)
> +Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
> +
> +The functionality was removed in Bug 1325503.
> +

Am I correct in inferring that these tests simply checked that these were *parseable values*? (and not that they were ever actually enabled)  i.e. these tests might as well have had "foobar" instead of "windows-xp" and would have behaved just the same?

If so, that might be worth noting in the extended commit message here.  Otherwise, it's a bit mysterious. ("hey why didn't these tests start failing when the functionality was removed way back in that other bug")
Attachment #8887981 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #13)
> drive-by -- typo:  "Remoev"

Thx - just noticed that myself. :)
(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8887981 [details]
> Bug 1330146. Remove windows-xp and windows-vista as values for
> -moz-os-version from tests.
> 
> https://reviewboard.mozilla.org/r/158850/#review164266
> 
> r=me, perhaps with a bit more detail in the extended commit message as noted
> below:
> 
> ::: commit-message-41349:1
> (Diff revision 1)
> > +Bug 1330146. Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
> > +
> > +The functionality was removed in Bug 1325503.
> > +
> 
> Am I correct in inferring that these tests simply checked that these were
> *parseable values*? (and not that they were ever actually enabled)  i.e.
> these tests might as well have had "foobar" instead of "windows-xp" and
> would have behaved just the same?

Yeah, it seems so. Which explains why they didn't start failing once we actually removed support for windows-xp and windows-vista values. I thought about leaving them in, but someone might be more confused in the future... 

> If so, that might be worth noting in the extended commit message here. 
> Otherwise, it's a bit mysterious. ("hey why didn't these tests start failing
> when the functionality was removed way back in that other bug")

Sounds good, will update.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8887650 - Flags: review?(bwinton) → review?(mwein)
Oh god, I am so the wrong person to review this!  Redirecting to Matt, who (I believe) has been working on WebExtension styling stuff recently… 
I have, but I haven't worked with extension-win-panel.css, so I think Kris would be better to review this.

Updated

7 months ago
Attachment #8887650 - Flags: review?(mwein) → review?(kmaglione+bmo)
Comment on attachment 8887650 [details]
Bug 1330146. Remove windows-xp and windows-vista values from extension-win-panel.css.

https://reviewboard.mozilla.org/r/158540/#review164292
Attachment #8887650 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks everyone!

Comment 26

7 months ago
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49f30c8fefa
Remove windows-xp and windows-vista values from extension-win-panel.css. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b626aca18e66
Remove windows-xp and windows-vista as values for -moz-os-version from tests. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/f49f30c8fefa
https://hg.mozilla.org/mozilla-central/rev/b626aca18e66
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.