Closed Bug 1411210 Opened 7 years ago Closed 6 years ago

Replace @media not all and (-moz-...) with @media (-moz-...: 0)

Categories

(Firefox :: Theme, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: dao, Assigned: vashishthmayur, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=css])

Attachments

(4 files)

+++ This bug was initially created as a clone of Bug #1313830 +++

We currently use "@media not all and (-moz-...)" with -moz-mac-yosemite-theme, -moz-windows-classic, and -moz-touch-enabled:

http://searchfox.org/mozilla-central/search?q=%40media+not+all+and+(-moz&case=true&regexp=false&path=

These can be simplified to "@media (-moz-...: 0)" like here:

http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/browser/themes/windows/browser.css#515
Hi, I would like to work on this as my beginning bugs.
Dão: Any pointers as to where I should start?
I have already set up the dev environment.
Flags: needinfo?(dao+bmo)
(In reply to Manish Kumar from comment #1)
> Hi, I would like to work on this as my beginning bugs.
> Dão: Any pointers as to where I should start?

Hi, start with the first link from comment 0. It shows you what files and lines you need to change.
Flags: needinfo?(dao+bmo)
Dão: Thanks for this info.

I would like to work on this. 
can you assign this bug to me?
Sure :)
Assignee: nobody → 1991manish.kumar
Are you still going to work on this? Let me know if you need help.
Flags: needinfo?(1991manish.kumar)
(In reply to Dão Gottwald [::dao] from comment #5)
> Are you still going to work on this? Let me know if you need help.

Thanks Dão, I will work on this bug, actually I was working on 1 more bug earlier and some unexpected error is coming in that. Once I am done with that bug, I will work on this.
Flags: needinfo?(1991manish.kumar)
Any update?
Flags: needinfo?(1991manish.kumar)
I would like to work on this bug. Please assign it to me.
Okay.
Assignee: 1991manish.kumar → vashishthmayur
Flags: needinfo?(1991manish.kumar)
1 more thing. I would like to know where is the source code repository. Couldn't find it. Sorry for such rookie question.
(In reply to Mayur Vashishth from comment #10)
> 1 more thing. I would like to know where is the source code repository.
> Couldn't find it. Sorry for such rookie question.

It's at <https://hg.mozilla.org/mozilla-central>. See also <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial>.
I have started working on this bug, but I am not sure if you I need to replace the ones that are missing the "not all" part. You have specified only the lines that that are "@media not all and...., but there are some that I found with the same parameters you need changed that are lacking the "not all part". I will assume they need to be changed as well, if not I will fix it.
Flags: needinfo?(vashishthmayur)
(In reply to Kristiyan from comment #12)
> I have started working on this bug, but I am not sure if you I need to
> replace the ones that are missing the "not all" part. You have specified
> only the lines that that are "@media not all and...., but there are some
> that I found with the same parameters you need changed that are lacking the
> "not all part". I will assume they need to be changed as well, if not I will
> fix it.

Only those with "not all and" need to be replaced. And the queried feature shouldn't change, e.g. "@media not all and (-moz-mac-yosemite-theme)" should become "@media (-moz-mac-yosemite-theme: 0)" rather than "@media (-moz-windows-default-theme: 0)".
Flags: needinfo?(vashishthmayur)
Sorry for the misunderstanding, I will redo it in the correct way. Thank you.
The patch fails to apply in some of the patched files. Are you working on mozilla-central? Is your copy up to date?
I reinstalled my VM yesterday evening and updated it as well, should be. Should I update it again and recommit?
(In reply to Kristiyan from comment #19)
> I reinstalled my VM yesterday evening and updated it as well, should be.
> Should I update it again and recommit?

Yeah, please do. Thanks!
Attached file media_test.txt
Updated my build, re-did the change set and GREPed for any leftovers in ../toolkit/themes and ../browser/themes.
Attachment #8936481 - Flags: feedback+
Comment on attachment 8935600 [details]
[mq]: Bug 1411210 - Update / Replace @media not all and (-moz-...)

https://reviewboard.mozilla.org/r/206482/#review213088

::: toolkit/themes/mobile/mozapps/plugins/pluginProblem.css:93
(Diff revision 3)
>  .msgTapToPlay,
>  .msgClickToPlay {
>    text-decoration: underline;
>  }
>  
> -@media not all and (-moz-touch-enabled) {
> +@media (-moz-touch-enabled) {

This should be @media (-moz-touch-enabled: 0) {

Looks good otherwise!
Fixed that, sorry I missed it, thank you for the support you've been very helpful.
Attachment #8935600 - Flags: review+
Comment on attachment 8936581 [details]
[mq]: Bug 1411210 - Final Touches / Replace @media not all and (-moz-...)

Thanks! Next time please fold modifications into your original patch so that we have one patch to land rather than two.
Attachment #8936581 - Flags: review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae78c4fe253a
Replace @media not all and (-moz-...) with @media (-moz-...: 0). r=dao
https://hg.mozilla.org/mozilla-central/rev/ae78c4fe253a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Too late for Beta58. Mark 58 won't fix.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: