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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ntim, Assigned: aljullu)

Tracking

(Blocks 1 bug)

Trunk
Firefox 34
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Updated

5 years ago
Blocks: 916766
OS: Windows 8.1 → All
Hardware: x86_64 → All
Where exactly do you want border-image to replace background-image ? They dont even serve the same purpose.
Reporter

Comment 2

5 years ago
(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/
Reporter

Updated

5 years ago
Flags: needinfo?(bgrinstead)
Summary: DevTools themes - Use border-image instead of background-image for DevTools UI → DevTools themes - Use border-image instead of background-image for separators and borders
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?
Flags: needinfo?(bgrinstead)
Reporter

Comment 4

5 years ago
(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.
Reporter

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Comment 5

5 years ago
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.
Assignee: nobody → aljullu
Reporter

Comment 6

5 years ago
(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 :)
Status: NEW → ASSIGNED
Assignee

Comment 7

5 years ago
Posted patch bug975804.patch (obsolete) — Splinter Review
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?
Attachment #8473769 - Flags: feedback?(bgrinstead)
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?
Attachment #8473769 - Flags: feedback?(bgrinstead) → feedback+
Reporter

Comment 9

5 years ago
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.
(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
(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
Reporter

Comment 12

5 years ago
(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.
(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/
Reporter

Comment 14

5 years ago
You can use this CSS too :
:root {
  direction: rtl !important;
}
Assignee

Comment 15

5 years ago
Posted patch bug975804b.patch (obsolete) — Splinter Review
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.
Attachment #8473769 - Attachment is obsolete: true
Attachment #8474231 - Flags: review?(bgrinstead)
Reporter

Comment 17

5 years ago
(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).
Assignee

Comment 18

5 years ago
(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.
(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 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 >
Attachment #8474231 - Flags: review?(bgrinstead) → review+
Assignee

Comment 22

5 years ago
Done! :)
Attachment #8474231 - Attachment is obsolete: true
Attachment #8479322 - Flags: review?(bgrinstead)
Attachment #8479322 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
Version: 30 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/1a58ce6615fb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Flags: qe-verify-

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.