975804 - DevTools themes - Use border-image instead of background-image for separators and borders

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Tim Nguyen :ntim]

Now that border image is supported for linear-gradients, I see no reason using background-image instead of border-image.

Comment 1

[Girish Sharma [:Optimizer]]

Where exactly do you want border-image to replace background-image ? They dont even serve the same purpose.

Comment 2

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #1)
> Where exactly do you want border-image to replace background-image ? They
> dont even serve the same purpose.

For the sidebar tabs for example. They use background-image to create their border.
Here's a demo I put up : http://jsfiddle.net/ntim/pXQXZ/

Comment 3

[Brian Grinstead [:bgrins]]

This seems like a nice use of border image since we are emulating a border with a second background.  Tim, would you like to work on this?

Comment 4

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> This seems like a nice use of border image since we are emulating a border
> with a second background.  Tim, would you like to work on this?

Sure, but I can't promise you anything since I'm quite busy with school already.

Comment 5

[Albert Juhé]

Tim, if it's ok I will assign this bug to me, since I have some time these days and some of the code using background-image was written by me.

Comment 6

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #5)
> Tim, if it's ok I will assign this bug to me, since I have some time these
> days and some of the code using background-image was written by me.
Go ahead :)

Comment 7

[Albert Juhé]

Attachment: bug975804.patch

This patch replaces background-image with border-image in toolbar.inc.css. If I'm not wrong, the files:
- computedview.css
- highlighter.inc.css
- ruleview.css
are also using background-image in places where it could be replaced with border-image. I'm will take a look at them in the following days.

By the way, is it better if I create a different patch for each one of them or one single patch for all four files?

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8473769 [details] [diff] [review]
bug975804.patch

Review of attachment 8473769 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't actually applied the patch to check it out yet, but it does look a lot cleaner.

As for whether to create a different patch for each file or a single patch for all four files I'd say go with a single patch

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +390,5 @@
>    text-shadow: none;
>  }
>  
> +.devtools-sidebar-tabs:-moz-locale-dir(rtl) > tabs > tab {
> +  background-width: 0 1px 0 0;

should this be border-width?

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8473769 [details] [diff] [review]
bug975804.patch

Review of attachment 8473769 [details] [diff] [review]:
-----------------------------------------------------------------

Also, I think it's better to use -moz-border-start: 1px rather than having a special RTL rule.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #9)
> Comment on attachment 8473769 [details] [diff] [review]
> bug975804.patch
> 
> Review of attachment 8473769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, I think it's better to use -moz-border-start: 1px rather than having a
> special RTL rule.

That's a good point.  Also, you can use this extension to quickly switch to RTL for testing: developer.mozilla.org/en/docs/Force_RTL

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > Comment on attachment 8473769 [details] [diff] [review]
> > bug975804.patch
> > 
> > Review of attachment 8473769 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Also, I think it's better to use -moz-border-start: 1px rather than having a
> > special RTL rule.
> 
> That's a good point.  Also, you can use this extension to quickly switch to
> RTL for testing: developer.mozilla.org/en/docs/Force_RTL

Probably would want -moz-border-start-width: 1px in this case

Comment 12

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > (In reply to Tim Nguyen [:ntim] from comment #9)
> > > Comment on attachment 8473769 [details] [diff] [review]
> > > bug975804.patch
> > > 
> > > Review of attachment 8473769 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Also, I think it's better to use -moz-border-start: 1px rather than having a
> > > special RTL rule.
> > 
> > That's a good point.  Also, you can use this extension to quickly switch to
> > RTL for testing: developer.mozilla.org/en/docs/Force_RTL
> 
> Probably would want -moz-border-start-width: 1px in this case

Yep :)
Btw, that mdn page has been deleted.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #11)
> > (In reply to Brian Grinstead [:bgrins] from comment #10)
> > > (In reply to Tim Nguyen [:ntim] from comment #9)
> > > > Comment on attachment 8473769 [details] [diff] [review]
> > > > bug975804.patch
> > > > 
> > > > Review of attachment 8473769 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > Also, I think it's better to use -moz-border-start: 1px rather than having a
> > > > special RTL rule.
> > > 
> > > That's a good point.  Also, you can use this extension to quickly switch to
> > > RTL for testing: developer.mozilla.org/en/docs/Force_RTL
> > 
> > Probably would want -moz-border-start-width: 1px in this case
> 
> Yep :)
> Btw, that mdn page has been deleted.

Here is the actual link: https://addons.mozilla.org/en-us/firefox/addon/force-rtl/

Comment 14

[Tim Nguyen :ntim]

You can use this CSS too :
:root {
  direction: rtl !important;
}

Comment 15

[Albert Juhé]

Attachment: bug975804b.patch

Ok, re-checking my list:
- toolbar.inc.css: I made the changes you said, thank you for the suggestions!
- highlighter.inc.css: The background-image is from .highlighter-nodeinfobar-arrow, I think that's no longer used, right? Should I fill a bug to remove that bunch of CSS?
- ruleview.css: That background-image comes from bug 956044 and it doesn't look like it's replaceable.
- computedview.css: That background-image is like the one from ruleview.css, so I don't think we can replace it with border-image either.

So, am I missing something or the only background-image that could be replaced with border-image was in toolbar.inc.css? I checked all the files from /themes/shared/devtools and I didn't see any other place.

Comment 16

[Albert Juhé]

Attachment: Screenshot from 2014-08-17 17:25:27.png

Comment 17

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #15)
> Created attachment 8474231 [details] [diff] [review]
> bug975804b.patch
> - highlighter.inc.css: The background-image is from
> .highlighter-nodeinfobar-arrow, I think that's no longer used, right? Should
> I fill a bug to remove that bunch of CSS?
It is still used, it's actually the arrow that points the element from the infobar. But I think there's actually a bug here, the arrow color doesn't match the info bar color, it's very subtle, but noticeable (hsl(210,2%,22%) should be hsl(214,13%,24%)). So yeah, I guess you could file a bug about that, or address that here if Brian is ok with it.


(In reply to Albert Juhé from comment #16)
> Created attachment 8474232 [details]
> Screenshot from 2014-08-17 17:25:27.png
For some reason, it seems that the small separators are a bit lighter than they currently are. But that might just be because I'm using Windows. I actually verified the colors using the eyedropper, they are actually different (#5A6169 for me, #5F7387 in the screenshot).

Comment 18

[Albert Juhé]

(In reply to Tim Nguyen [:ntim] from comment #17)
> It is still used, it's actually the arrow that points the element from the
> infobar. But I think there's actually a bug here, the arrow color doesn't
> match the info bar color, it's very subtle, but noticeable (hsl(210,2%,22%)
> should be hsl(214,13%,24%)). So yeah, I guess you could file a bug about
> that, or address that here if Brian is ok with it.

Oh, true. I will fill a new bug then.

(In reply to Tim Nguyen [:ntim] from comment #17)
> (In reply to Albert Juhé from comment #16)
> > Created attachment 8474232 [details]
> > Screenshot from 2014-08-17 17:25:27.png
> For some reason, it seems that the small separators are a bit lighter than
> they currently are. But that might just be because I'm using Windows. I
> actually verified the colors using the eyedropper, they are actually
> different (#5A6169 for me, #5F7387 in the screenshot).

Yeah, sorry. Actually the colors from the screenshot are wrong, because when I took it I had the patch from bug 1049012 applied.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #18)
> (In reply to Tim Nguyen [:ntim] from comment #17)
> > It is still used, it's actually the arrow that points the element from the
> > infobar. But I think there's actually a bug here, the arrow color doesn't
> > match the info bar color, it's very subtle, but noticeable (hsl(210,2%,22%)
> > should be hsl(214,13%,24%)). So yeah, I guess you could file a bug about
> > that, or address that here if Brian is ok with it.
> 
> Oh, true. I will fill a new bug then.

Yes, let's do that in a new bug.

> Ok, re-checking my list:
> - toolbar.inc.css: I made the changes you said, thank you for the
> suggestions!
> - highlighter.inc.css: The background-image is from
> .highlighter-nodeinfobar-arrow, I think that's no longer used, right? Should
> I fill a bug to remove that bunch of CSS?
> - ruleview.css: That background-image comes from bug 956044 and it doesn't
> look like it's replaceable.
> - computedview.css: That background-image is like the one from ruleview.css,
> so I don't think we can replace it with border-image either.
> 
> So, am I missing something or the only background-image that could be
> replaced with border-image was in toolbar.inc.css? I checked all the files
> from /themes/shared/devtools and I didn't see any other place.

Sounds reasonable.  I think that updating toolbars.inc.css will be a good target for this bug

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8474231 [details] [diff] [review]
bug975804b.patch

Review of attachment 8474231 [details] [diff] [review]:
-----------------------------------------------------------------

Much cleaner CSS!  Please change commit message to clarify that it is just for sidebar tabs and add r=bgrins

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +391,4 @@
>    text-shadow: none;
>  }
>  
> +.devtools-sidebar-tabs> tabs > tab:first-child {

nit: space before >

Comment 21

[Brian Grinstead [:bgrins]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=735024c55f89

Comment 22

[Albert Juhé]

Attachment: bug975804c.patch

Done! :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1a58ce6615fb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a58ce6615fb