Closed Bug 1289848 Opened 3 years ago Closed 3 years ago

Download toolbar button reverts to lodpi when pressed

Categories

(Firefox :: Theme, defect, P2)

All
Linux
defect

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox50 + fixed
firefox51 --- verified

People

(Reporter: eeejay, Assigned: mgsudhanva, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 6 obsolete files)

This is because indicator.css needs to be updated.

I took a look, and I think that 95% of that stylesheet can be unified across all platforms. I don't have time to take this on, but I think this would be a good mentored bug for someone else..
[Tracking Requested - why for this release]:
Visual regression for Linux users on hidpi machines

(I'd offer to mentor this myself but I'm away for the next two weeks. Maybe Dão, Mike or Jared would be interested?)
Blocks: 1280133
Keywords: regression
Priority: -- → P2
Dão, can you mentor someone? We're trying to adhere to our "ship with no regressions" policy now :)
We tolerated the low rez version for years. Is it really a regression?
Let's not worry about unifying the stylesheet across platforms for now, but just update browser/themes/linux/downloads/indicator.css to be DPI-aware. browser/themes/windows/downloads/indicator.css can be used as a guideline for this (see '@media not all and (min-resolution: 1.1dppx) {...}' and '@media (min-resolution: 1.1dppx) {...}').
Mentor: dao+bmo
Whiteboard: [good first bug][lang=css]
(In reply to Eitan Isaacson [:eeejay] from comment #3)
> We tolerated the low rez version for years. Is it really a regression?

The regression is arbitrarily switching to the low-DPI icon. Using low-DPI icons all the time was at least consistent.
Bug 1153529 fixed this on Windows, see attachment 8627220 [details] [diff] [review]. The patch for this bug should be very similar.
Has Regression Range: --- → yes
hello , i would like to work on this bug. How do i proceed ?
(In reply to mgsudhanva from comment #7)
> hello , i would like to work on this bug. How do i proceed ?

1. Get your copy of our code base, e.g. using 'hg clone https://hg.mozilla.org/mozilla-central/ src',
2. make your changes in browser/themes/linux/downloads/indicator.css, and
3. create the patch e.g. using 'hg diff > ~/mypatch.diff'.
(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to mgsudhanva from comment #7)
> > hello , i would like to work on this bug. How do i proceed ?
> 
> 1. Get your copy of our code base, e.g. using 'hg clone
> https://hg.mozilla.org/mozilla-central/ src',
> 2. make your changes in browser/themes/linux/downloads/indicator.css, and
> 3. create the patch e.g. using 'hg diff > ~/mypatch.diff'.

I am finished with setting up with environment , now in the file  browser/themes/linux/downloads/indicator.css what should i work on
(In reply to mgsudhanva from comment #10)
> I am finished with setting up with environment , now in the file 
> browser/themes/linux/downloads/indicator.css what should i work on

You need to modify it like browser/themes/windows/downloads/indicator.css was modified in attachment 8627220 [details] [diff] [review].
Attachment #8782400 - Flags: review?(dao+bmo)
Comment on attachment 8782400 [details] [diff] [review]
Bug 1289848 - Download toolbar button reverts to lodpi when pressed

>+@media not all and (min-resolution: 1.1dppx) {
>+    #downloads-indicator-icon {
>+      --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 0, 198, 18, 180);
>+      --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 18, 198, 36, 180);
>+     --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
>+      --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 18, 198, 36, 180);
>+    }
>+  }
>+  
>+  @media (min-resolution: 1.1dppx) {
>+    #downloads-indicator-icon {
>+      --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 0, 396, 36, 360);
>+      --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 36, 396, 72, 360);
>+      --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 0, 396, 36, 360);
>+      --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 36, 396, 72, 360);
>+    }
>+  }

Indentation is messed up here (all lines except the first one are indented too far)

Looks good otherwise!
Attachment #8782400 - Flags: review?(dao+bmo)
Indentation corrected
Attachment #8782400 - Attachment is obsolete: true
Attachment #8782458 - Flags: review?(dao+bmo)
Comment on attachment 8782458 [details] [diff] [review]
Bug 1289848 - Download toolbar button reverts to lodpi when pressed

This appears to be a patch on top of the previous one. We'll need a patch based on mozilla-central tip instead.
Attachment #8782458 - Flags: review?(dao+bmo)
The original patch with the indentation changes
Attachment #8782458 - Attachment is obsolete: true
Attachment #8782522 - Flags: review?(dao+bmo)
Comment on attachment 8782522 [details] [diff] [review]
Bug 1289848 - Download toolbar button reverts to lodpi when pressed

>+@media not all and (min-resolution: 1.1dppx) {
>+  #downloads-indicator-icon {
>+    --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 0, 198, 18, 180);
>+    --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 18, 198, 36, 180);
>+    --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
>+    --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 18, 198, 36, 180);
>+    }
>+  }

On Windows we're using #downloads-button for this, not #downloads-indicator-icon. We should probably do the same here.

The closing brackets are still indented too far.

>+@media (min-resolution: 1.1dppx) {
>+  #downloads-indicator-icon {
>+    --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 0, 396, 36, 360);
>+    --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 36, 396, 72, 360);
>+    --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 0, 396, 36, 360);
>+    --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 36, 396, 72, 360);
>+    }
>+  }

Same here.
Attachment #8782522 - Flags: review?(dao+bmo)
Assignee: nobody → mgsudhanva
Changed from #downloads-indicator-icon to #downloads-button
Attachment #8782522 - Attachment is obsolete: true
Attachment #8782547 - Flags: review?(dao+bmo)
Comment on attachment 8782547 [details] [diff] [review]
Bug 1289848 - Download toolbar button reverts to lodpi when pressed

>+@media not all and (min-resolution: 1.1dppx) {
>+  #downloads-button {
>+    --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 0, 198, 18, 180);
>+    --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 18, 198, 36, 180);
>+    --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
>+    --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 18, 198, 36, 180);
>+  }
>+}
>+  
>+@media (min-resolution: 1.1dppx) {

nit: the seemingly empty line between these blocks has two superfluous spaces

>+  #downloads-button {
>+    --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 0, 396, 36, 360);
>+    --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar@2x.png"), 36, 396, 72, 360);
>+    --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 0, 396, 36, 360);
>+    --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted@2x.png"), 36, 396, 72, 360);
>+   }
>+ }

nit: the two closing brackets are still indented too far by one space

> toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention="success"]) > #downloads-indicator-anchor > #downloads-indicator-icon {
>-  background: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"),
>-                              0, 198, 18, 180) center no-repeat;
>+  background-image: var(--downloads-indicator-icon-inverted);
>+
> }

nit: remove the empty line from this rule

Thanks!
Attachment #8782547 - Flags: review?(dao+bmo) → review+
Indentation corrected
Attachment #8782547 - Attachment is obsolete: true
Attachment #8783188 - Flags: review?(dao+bmo)
Comment on attachment 8783188 [details] [diff] [review]
Comment Bug 1289848 - Download toolbar button reverts to lodpi when pressed

>+@media not all and (min-resolution: 1.1dppx) {
>+ #downloads-button {
>+  --downloads-indicator-icon: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 0, 198, 18, 180);
>+  --downloads-indicator-icon-attention: -moz-image-rect(url("chrome://browser/skin/Toolbar.png"), 18, 198, 36, 180);
>+  --downloads-indicator-icon-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 0, 198, 18, 180);
>+  --downloads-indicator-icon-attention-inverted: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"), 18, 198, 36, 180);
>+  }
>+}

Now you've removed too many spaces before '#downloads-button {' and the CSS variable lines :(

> toolbar[brighttext] #downloads-button[cui-areatype="toolbar"]:not([attention="success"]) > #downloads-indicator-anchor > #downloads-indicator-icon {
>-  background: -moz-image-rect(url("chrome://browser/skin/Toolbar-inverted.png"),
>-  0, 198, 18, 180) center no-repeat;
>+  background-image: var(--downloads-indicator-icon-inverted);
>+
> }

Huh, here you seem to have modified the patch after creating it (removing spaces before the '0, 198, 18, 180) center no-repeat;' line) such that it won't apply anymore. And the blank line before } is still there.

Please don't mess with the patch file, just modify browser/themes/linux/downloads/indicator.css and then create a new patch.
Attachment #8783188 - Flags: review?(dao+bmo) → review-
Sorry , in the last patch , removing that space was by mistake. Instead if removing the empty line the space got removed . I have attached the old patch which has the proper changes . Sorry for confusion!
Attachment #8783188 - Attachment is obsolete: true
Attachment #8783197 - Flags: review?(dao+bmo)
Comment on attachment 8783197 [details] [diff] [review]
Bug 1289848 - Download toolbar button reverts to lodpi when pressed

Hmm, looks good at a first glance, but have you still manually modified the patch after creating it? hg import refuses to apply this patch and says the first hunk is broken.
Attachment #8783197 - Flags: review?(dao+bmo)
Attached patch Bug 1289848Splinter Review
New created patch
Attachment #8783197 - Attachment is obsolete: true
Attachment #8783201 - Flags: review?(dao+bmo)
Comment on attachment 8783201 [details] [diff] [review]
Bug 1289848

Thanks. There are still two trailing spaces in the empty line between the two media query blocks. I'll fix this before landing the patch.
Attachment #8783201 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06034e3599c3
Download toolbar button reverts to lodpi when pressed r=dao
For future patches, please use a commit message that describes the change (rather than one that describes the problem).  (Usually, the commit message should not simply be the same as the bug title.)

See this doc for more info:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

Thanks!
https://hg.mozilla.org/mozilla-central/rev/06034e3599c3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8783201 [details] [diff] [review]
Bug 1289848

Approval Request Comment
[Feature/regressing bug #]: bug 1280133
[User impact if declined]: see comment 5
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low. we've basically ported an older patch for Windows from bug 1153529
[String/UUID change made/needed]: none
Attachment #8783201 - Flags: approval-mozilla-aurora?
Tracked for 50+, not a recent regression but seems to affect linux users on high dpi monitors.
Comment on attachment 8783201 [details] [diff] [review]
Bug 1289848

CSS only, low-risk, Aurora50+
Hi Eitan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(eitan)
Attachment #8783201 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It works!
Status: RESOLVED → VERIFIED
Flags: needinfo?(eitan)
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.