Make the small swatch of current theme in the themes button smaller and rounded.

VERIFIED FIXED in Firefox 39

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: bwinton, Assigned: farhaan, Mentored)

Tracking

unspecified
Firefox 40
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39+ fixed, firefox40 verified)

Details

(Whiteboard: [qx:spec] [lang=css] [good first bug])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Having the swatch is a nice improvement, but it's a little too big.
https://dl.dropboxusercontent.com/u/2301433/ThemeSwatch.png

And while we're modifying the swatch style, it would be nice to have a 3 pixel border-radius, to match the button.  :)

Farhaan, you've worked in this code before; did you want to take a shot at quickly fixing this one?
(Reporter)

Comment 1

3 years ago
(Although, it looks like the border-radius doesn't do anything to the image, so that part might not be possible.)
(Assignee)

Comment 2

3 years ago
yeah sure !! with your guidance ;)
(Assignee)

Comment 3

3 years ago
assign it to me Blake , I would like to work on it for sure.
Assignee: nobody → farhaan.bukhsh
Status: NEW → ASSIGNED
(Reporter)

Comment 4

3 years ago
Awesome!  Do you remember where the css is from the previous swatch bug?
(Assignee)

Comment 5

3 years ago
yup i remember i guess

Comment 6

3 years ago
(In reply to Blake Winton (:bwinton) from comment #1)
> (Although, it looks like the border-radius doesn't do anything to the image,
> so that part might not be possible.)

overflow: hidden might work.
(Assignee)

Comment 7

3 years ago
Ok i will try and let you know, sorry for not updating this long !!

Comment 8

3 years ago
[Tracking Requested - why for this release]:

This is pretty visible on customize mode (esp. on retina where the image is also pixelly) and we should fix it before releasing 39.
status-firefox39: --- → affected
tracking-firefox39: --- → ?
tracking-firefox39: ? → +
(Assignee)

Comment 9

3 years ago
None of the css property is affecting the swatch, I will be glad if someone can lend me a bit help on this one.
I tried a lot of things link of which i will put up. Help required
(Assignee)

Comment 10

3 years ago
I tried a lot of stuff but nothing is affecting that swatch need help to resolve this.
List of things I tried:

https://pastebin.mozilla.org/8826870
Flags: needinfo?(jaws)

Updated

3 years ago
Flags: needinfo?(bwinton)
(Reporter)

Comment 11

3 years ago
Hey Farhaan,

Jared and I have both tried to get the rounded corners working, and they don't seem to be applying for either of us.  As a compromise for this version, can we just make the swatch smaller?

In the previous bug, you changed line 149 of browser/themes/shared/customizableui/customizeMode.inc.css to have:
  #customization-lwtheme-button  {
    padding: 2px 7px;
  }

  #customization-lwtheme-button > .box-inherit > .box-inherit > .button-icon {
    -moz-margin-end: 3px;
    height: 22px;
  }
I think this will probably do what we're asking for…

Thanks,
Blake.
Flags: needinfo?(bwinton)
We can probably get the border-radius if we use the clip-path CSS property and an SVG shape. Blake, would you like to look in to this?
Flags: needinfo?(jaws) → needinfo?(bwinton)
(Reporter)

Comment 13

3 years ago
I've got a bunch of reading list transition bugs to do first, but if no-one else picks it up before me, I'll take a stab at it.  (Also, is clip-path and SVG _really_ the best solution here?  Cause that's pretty gross, if it is.  ;)
Flags: needinfo?(bwinton)
MattN and Gijs were talking about an alternative approach here, http://logs.glob.uno/?c=mozilla%23fx-team&s=24+Mar+2015&e=24+Mar+2015&h=clip-path#c199079
(Assignee)

Comment 15

3 years ago
I guess this works fine please check and let me know if its good!!
(Assignee)

Comment 16

3 years ago
Created attachment 8583856 [details]
Screen Shot 2015-03-26 at 8.57.24 pm.png
(Assignee)

Comment 17

3 years ago
if that is what you need then its done in this way!
https://pastebin.mozilla.org/8827223
(Assignee)

Updated

3 years ago
Flags: needinfo?(jaws)
Flags: needinfo?(bwinton)
(Reporter)

Comment 18

3 years ago
I think that's better than what we currently have…  Philipp, do you think this new smaller size is good enough to ship?
Flags: needinfo?(bwinton) → needinfo?(philipp)
(Reporter)

Comment 19

3 years ago
Created attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…

Hey Farhaan, sorry for stealing the bug like this, but I had a bit of time this afternoon, so I did some hacking and got it looking like this: https://dl.dropboxusercontent.com/u/2301433/Firefox/ThemeButton.png

(I'm also redirecting the review request to Gijs, since this was his suggestion, and since Jaws isn't accepting review requests for a little while.  ;)
Attachment #8584680 - Flags: ui-review?(philipp)
Attachment #8584680 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 20

3 years ago
Hey no worries but how did you do it?
(Reporter)

Comment 21

3 years ago
Check the diff!  ;)

(Seriously, take a look at the diff, and see which parts make sense, and which don't, and ask me about the ones you don't understand…  If your question was more "Where did I get the idea?", then the answer is "From the logs Jaws posted in comment 14." :)
(Assignee)

Comment 22

3 years ago
Why the change in javascript part is needed? Now direct me to other bug :P
(Reporter)

Comment 23

3 years ago
The change in javascript is needed to set the background-image style on the sub-element, instead of setting the image attribute on the button.  (If we set the image attribute on the button, we would get two images, one of which wouldn't be rounded. :)

For the next bug, how do you feel about taking another stab at bug 1102710?  Or you could finish off the work NTim started in bug 1111094

Updated

3 years ago
Attachment #8584680 - Flags: review?(gijskruitbosch+bugs) → review+
Flags: needinfo?(jaws)
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…

I didn't apply the patch, but I assume that it looks like the buttons in bwintons screenshots (https://dl.dropboxusercontent.com/u/2301433/Firefox/ThemeButton.png).

That's a definite improvement!
Flags: needinfo?(philipp)
Attachment #8584680 - Flags: ui-review?(philipp) → ui-review+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/012456b15383
Keywords: checkin-needed
Whiteboard: [qx] [lang=css] [good first bug] → [qx] [lang=css] [good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/012456b15383
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [qx] [lang=css] [good first bug][fixed-in-fx-team] → [qx] [lang=css] [good first bug]
Target Milestone: --- → Firefox 40
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…

Approval Request Comment
[Feature/regressing bug #]: issue with new feature introduced in bug 1073234
[User impact if declined]: customization mode looks a little uglier on the themes button
[Describe test coverage new/current, TreeHerder]: manual testing, no automated coverage, just a styling change
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8584680 - Flags: approval-mozilla-aurora?
Comment on attachment 8584680 [details] [diff] [review]
Let's just make the corners rounded…

Approving this for aurora. Round away.
Attachment #8584680 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb9ade69b666
status-firefox39: affected → fixed
(Reporter)

Updated

3 years ago
Whiteboard: [qx] [lang=css] [good first bug] → [qx:spec] [lang=css] [good first bug]

Comment 30

2 years ago
I have successfully reproduce this bug on Firefox nightly 39.0a1 (2015-03-05)

The bug's fix is verified on latest release 40.0 ( build id : 20150807085045)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
QA Whiteboard: [bugday-20150812]

Updated

2 years ago
Status: RESOLVED → VERIFIED

Updated

2 years ago
status-firefox40: fixed → verified
You need to log in before you can comment on or make changes to this bug.