1100443 - DevTools Themes: Make infobar arrow smaller

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Albert Juhé]

From Bug 1059376 Comment 4:
> since the infobar font-size has been made smaller. It makes sense to me to make the arrow size smaller as well.

Do we have any spec or mockup or should I just try different sizes and decide which one looks better?

Comment 1

[Tim Nguyen :ntim]

I would try different sizes first. It'd be nice to use em units btw, so the arrow size will be proportional to the font-size for all cases.

Comment 2

[Albert Juhé]

Attachment: arrow-size.png

Here are some examples, from 0.6em to 1.0em. Which one do you like most?

Comment 3

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #2)
> Created attachment 8550972 [details]
> arrow-size.png
> 
> Here are some examples, from 0.6em to 1.0em. Which one do you like most?

0.7em looks good to me.

Comment 4

[Albert Juhé]

I saw there is a problem using em instead of px. On /toolkit/devtools/server/actors/highlighter.js the arrow size is used to correctly position the infobar:
http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#30
http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#1283

Any advice in how to solve that?

Comment 5

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #4)
> I saw there is a problem using em instead of px. On
> /toolkit/devtools/server/actors/highlighter.js the arrow size is used to
> correctly position the infobar:
> http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> highlighter.js#30
> http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> highlighter.js#1283
> 
> Any advice in how to solve that?

getComputedStyle ? Patrick will now better than me for this.

Comment 6

[Patrick Brosset <:pbro>]

The size of the arrow is defined in this CSS rule: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.css?force=1#97

If you want to change it to .7em, you'll have to change these 2 properties:
left: calc(50% - 14px);
and
border: 14px solid hsl(214,13%,24%);
change 14px by .7em

Now, the NODE_INFOBAR_ARROW_SIZE const in highlighter.js here: http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#30 is used to make sure the height of the arrow doesn't cover up the node.

The bad news is there isn't a good way now to calculate this height dynamically because the highlighter DOM lives in a native anonymous node in one of the document root frames, and there's any way to access it from chrome JS.

So that means whatever height we choose in the CSS, we must have the corresponding height as a const in the JS.
In that case, why choose ems at all? The font-size of the nodeinfobar isn't supposed to change anyway, right? Why not settle for something like 8px (which looks good to me) and put that in both places.

Also, adding a comment above |const NODE_INFOBAR_ARROW_SIZE = 8; // px| explaining why this height has to be hard-coded here would help.

Comment 7

[Albert Juhé]

Attachment: bug1100443.patch

Done! If I'm not missing anything, this patch should work. I used a sentence from Patrick Brosset's explanation as a comment on highlighter.js.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: with-patch-applied.png

With the patch applied, I'm seeing a gap between the arrow and the node

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: without-patch-applied.png

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8551410 [details] [diff] [review]
bug1100443.patch

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

::: toolkit/devtools/server/actors/highlighter.css
@@ -94,1 @@
>  

There is a line up here:


:-moz-native-anonymous .box-model-nodeinfobar-container[hide-arrow] > .box-model-nodeinfobar {
  margin: 7px 0;
}

I have a hunch (but haven't tested) that this needs to be updated to 4px 0 to get rid of the space seen in Comment 8

Comment 11

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> I have a hunch (but haven't tested) that this needs to be updated to 4px 0
> to get rid of the space seen in Comment 8

Or even 0.35em 0, if that's possible.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #11)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > I have a hunch (but haven't tested) that this needs to be updated to 4px 0
> > to get rid of the space seen in Comment 8
> 
> Or even 0.35em 0, if that's possible.

I think everything is being switched to px as per Comment 6.  I don't see a reason not to do that.

Comment 13

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Tim Nguyen [:ntim] from comment #11)
> > (In reply to Brian Grinstead [:bgrins] from comment #10)
> > > I have a hunch (but haven't tested) that this needs to be updated to 4px 0
> > > to get rid of the space seen in Comment 8
> > 
> > Or even 0.35em 0, if that's possible.
> 
> I think everything is being switched to px as per Comment 6.  I don't see a
> reason not to do that.

Oh ok, didn't read the end of the comment.

Comment 14

[Albert Juhé]

Attachment: bug1100443b.patch

Wow, I didn't see the gap. Actually the problem was that I didn't update the NODE_INFOBAR_HEIGHT variable too (I didn't know the arrow size had to be included). This patch fixes that.

Comment 15

[Albert Juhé]

Attachment: margin.png

By the way, the margin you said:

> :-moz-native-anonymous .box-model-nodeinfobar-container[hide-arrow] > .box-model-nodeinfobar {
>   margin: 7px 0;
> }

Is used when no arrow is displayed. I'm not sure if it must have some correlation with the arrow size. I didn't modify it because I was not sure.

Comment 16

[Albert Juhé]

Attachment: bug1100443c.patch

(sorry for the multiple emails)

I saw that originally there was a small 1px gap between the arrow and the element, I added it back in this patch. In the previous patch there were also some wrong values.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8552674 [details] [diff] [review]
bug1100443c.patch

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

Looks good to me - pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baebbb3db62b

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfd8a15f50b2