868163 - Highlight the Debugger's tab when it gets paused and is not the current tool.

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

When the debugger gets paused, and the current tool is not the debugger, there should be a visual clue that debugger paused. We can do it the way Chrome does it (blocking the page and saying that debugger is paused), but it will render our Inspector to fail (which still is usable using the markup view).

While this issue was fixed for Inspector, there can be a generalized API to tell the Toolbox that xyz tool needs attention, and we can highlight the tab of xyz tool.
Or, we can just special case debugger and highlight its tab on paused notification.

Comment 1

[Girish Sharma [:Optimizer]]

Okay, so I propose a new public method in Toolbox:
toolbox.highlightTool(toolId).

Toolbox will check if the tool with toolId is the current tool or not. If it is the current tool, then do nothing. If not, then highlight the tool with a different color (I am thinking orange-ish). As soon as the tool that is highlighted is selected, its highlight will go off.

I don't know if toolbox.dehighlightTool(toolId) will make sense or not.

Any thoughts anyone ?

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Initial patch. Asking for an open feedback, if anyone sees, this that is.

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Somewhat better animation. (personal opinion)

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Switching to classes.
Not Flashing anymore :(

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 746050 [details] [diff] [review]
patch v0.3

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

::: browser/devtools/debugger/DebuggerPanel.jsm
@@ +54,5 @@
>        .then(() => this._controller.startupDebugger())
>        .then(() => this._controller.connect())
>        .then(() => {
> +        this.target.on("thread-paused", this.highlightWhenPaused);
> +        this.target.on("thread-resumed", this.dehighlightWhenResumed);

I'm not sure if these belong here or in the controller. Deferring (heh) to Victor.

::: browser/themes/osx/devtools/toolbox.css
@@ +229,5 @@
> +  color: #e9692c;
> +  background-image: radial-gradient(farthest-corner at center top, #e9692c, hsla(31,92%,75%,.3)),
> +                    radial-gradient(farthest-side at center top, hsla(31,92%,75%,.4), hsla(31,92%,75%,0)),
> +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> +                    linear-gradient(hsla(18, 42%, 37%, .02), hsla(204,45%,98%,.04));

I'm not sure about these color values. Probably OK, but I'd like shorlander to take a look. See attached.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: Screenshot OS X

Stephen, could you take a look at these colors? To my eye, the orange on dark blue looks a little funny. The contrast isn't quite right.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 746050 [details] [diff] [review]
patch v0.3

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

The controller doesn't have access to the toolbox, so the methods are ok where they are.
However, this patch needs tests.
Also, IMHO, that orange/dark background combination is hideous.

Comment 8

[Victor Porof [:vporof][:vp]]

Also, I don't understand why we need to Highlight the debugger's tab. We already have a notification box which essentially does the same thing no? I honestly think we should WONTFIX this, but I can be convinced otherwise :)

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #8)
> Also, I don't understand why we need to Highlight the debugger's tab. We
> already have a notification box which essentially does the same thing no? I
> honestly think we should WONTFIX this, but I can be convinced otherwise :)

Right now the notification box is just for the Inspector tool. That notification was introduced to tell the user that Mouse events would nto work to select any element. The only other option to tell user that thing was to dim the page, which ofcourse was not a very good option, thus notification box was chosen.

My reasoning on why this approach is better than a notification box on each tool when debugger is paused:

1) This is a public API , so any tool can use it when needed.
2) The notification box is too heavy UI, it takes up 100% width and quite a bit of height too, not to mention that it pushes the toolbox down. So, just to say that "Debugger is paused", adding a notification box seems like Overkill.
3) The notification box could be closed, causes reflow of the whole toolbox while opening and closing. Not to mention that if you are doing something on other tool, sudden vertical movement would hinder your workflow.
4) A highlight on the tab is neither too alarming, nor too light; It is just the perfect way to tell user that this tool needs attention.

All the above views are my personal thoughts and a start to an open discussion :)

Comment 10

[Girish Sharma [:Optimizer]]

Oh, and I forgot :

5) Firebug also does something similar, so people using firebug for debugging would be used to such a feature.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Updated the color to use the same color as the ":hover" text in breadcrumbs of Inspector.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: screenshot of patch v0.4 on Windows 7

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 748163 [details] [diff] [review]
patch v0.4

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

I just passed my laptop around, and the consensus is that it would look better without changing the text color. Just the orange bar should be enough.

r+ with these changes.

::: browser/themes/linux/devtools/toolbox.css
@@ +224,5 @@
>    box-shadow: 1px -1px 0 hsla(206,37%,4%,.2) inset;
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {
> +  color: hsl(20,100%,70%);

lose the "color" rule.

::: browser/themes/osx/devtools/toolbox.css
@@ +211,5 @@
>    box-shadow: 1px -1px 0 hsla(206,37%,4%,.2) inset;
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {
> +  color: hsl(20,100%,70%);

lose color rule.

@@ +212,5 @@
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {
> +  color: hsl(20,100%,70%);
> +  background-image: radial-gradient(farthest-corner at center top, hsl(20,100%,70%), hsla(20,100%,70%,.3)),

should be: 20,100%,85% on OS X.

::: browser/themes/windows/devtools/toolbox.css
@@ +220,5 @@
>    box-shadow: 1px -1px 0 hsla(206,37%,4%,.2) inset;
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {
> +  color: hsl(20,100%,70%);

remove.

@@ +221,5 @@
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {
> +  color: hsl(20,100%,70%);
> +  background-image: radial-gradient(farthest-corner at center top, hsl(20,100%,70%), hsla(20,100%,70%,.3)),

ok here.

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5 with tests

addressed review comments, added one debugger test and one framework test.
Carrying forward r+ from Rob.
Asking review from Victor for the tests.

Green try at : https://tbpl.mozilla.org/?tree=Try&rev=d35d795e703d

Comment 15

[Girish Sharma [:Optimizer]]

Attachment: screenshot of v0.5 on windows 7

Now only adding background orange and not changing the text color.

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 751437 [details] [diff] [review]
patch v0.5 with tests

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

r+ with the changes below.

::: browser/devtools/debugger/test/browser_dbg_bug868163_highight_on_pause.js
@@ +16,5 @@
> +  debug_tab_pane(STACK_URL, function(aTab, aDebuggee, aPane) {
> +    gTab = aTab;
> +    gDebugger = aPane.panelWin;
> +    var target = TargetFactory.forTab(gTab);
> +    gToolbox = gDevTools.getToolbox(target);

gToolbox = aPane._toolbox. No need for target.

::: browser/devtools/framework/test/browser_toolbox_highlight.js
@@ +22,5 @@
> +             .then(aToolbox => {
> +                toolbox = aToolbox;
> +                // select tool 2
> +                toolbox.selectTool(TOOL_ID_2)
> +                       // and highlight the first one

This is some funky indenting. Are these tabs? It looks like 8 spaces.

@@ +35,5 @@
> +                       // Switch to tool 2 again
> +                       .then(() => toolbox.selectTool(TOOL_ID_2))
> +                       // and check again.
> +                       .then(checkHighlighted.bind(null, TOOL_ID_1))
> +                       // No wdehighlight the tool

wdehighlight?

@@ +39,5 @@
> +                       // No wdehighlight the tool
> +                       .then(dehighlightTab.bind(null, TOOL_ID_1))
> +                       // to see the classes gone.
> +                       .then(checkNoHighlight.bind(null, TOOL_ID_1))
> +                       // Now close the toolbox and exit. 

Trailing whitespace.

::: browser/themes/linux/devtools/toolbox.css
@@ +231,5 @@
> +.devtools-tab:not([selected=true]).highlighted {
> +  background-image: radial-gradient(farthest-corner at center top, hsl(20,100%,70%), hsla(20,100%,70%,.3)),
> +                    radial-gradient(farthest-side at center top, hsla(20,100%,70%,.4), hsla(20,100%,70%,0)),
> +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> +                    linear-gradient(hsla(18, 42%, 37%,.02), hsla(204,45%,98%,.04));

Nit: no spaces after commas in hlsl()

::: browser/themes/osx/devtools/toolbox.css
@@ +213,5 @@
>    background-position: top right, top left, left, right;
>    box-shadow: 1px -1px 0 hsla(206,37%,4%,.2) inset;
>  }
>  
> +.devtools-tab:not([selected=true]).highlighted {\

Dude. Why the backslash?

::: browser/themes/windows/devtools/toolbox.css
@@ +226,5 @@
> +.devtools-tab:not([selected=true]).highlighted {
> +  background-image: radial-gradient(farthest-corner at center top, hsl(20,100%,70%), hsla(20,100%,70%,.3)),
> +                    radial-gradient(farthest-side at center top, hsla(20,100%,70%,.4), hsla(20,100%,70%,0)),
> +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> +                    linear-gradient(hsla(18, 42%, 37%,.02), hsla(204,45%,98%,.04));

Nit: no spaces after commas in hlsl()

Comment 17

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #16)
> Comment on attachment 751437 [details] [diff] [review]
> patch v0.5 with tests
> 
> Review of attachment 751437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the changes below.
> 
> ::: browser/devtools/debugger/test/browser_dbg_bug868163_highight_on_pause.js
> @@ +16,5 @@
> > +  debug_tab_pane(STACK_URL, function(aTab, aDebuggee, aPane) {
> > +    gTab = aTab;
> > +    gDebugger = aPane.panelWin;
> > +    var target = TargetFactory.forTab(gTab);
> > +    gToolbox = gDevTools.getToolbox(target);
> 
> gToolbox = aPane._toolbox. No need for target.

Ah..

> ::: browser/devtools/framework/test/browser_toolbox_highlight.js
> @@ +22,5 @@
> > +             .then(aToolbox => {
> > +                toolbox = aToolbox;
> > +                // select tool 2
> > +                toolbox.selectTool(TOOL_ID_2)
> > +                       // and highlight the first one
> 
> This is some funky indenting. Are these tabs? It looks like 8 spaces.

No, those are spaces only. I am trying to indent with the . see the whole paragraph https://bugzilla.mozilla.org/attachment.cgi?id=751437&action=diff#a/browser/devtools/framework/test/browser_toolbox_highlight.js_sec2


> @@ +35,5 @@
> > +                       // Switch to tool 2 again
> > +                       .then(() => toolbox.selectTool(TOOL_ID_2))
> > +                       // and check again.
> > +                       .then(checkHighlighted.bind(null, TOOL_ID_1))
> > +                       // No wdehighlight the tool
> 
> wdehighlight?

Spac eissue s.
 
> ::: browser/themes/osx/devtools/toolbox.css
> @@ +213,5 @@
> >    background-position: top right, top left, left, right;
> >    box-shadow: 1px -1px 0 hsla(206,37%,4%,.2) inset;
> >  }
> >  
> > +.devtools-tab:not([selected=true]).highlighted {\
> 
> Dude. Why the backslash?

I don't even.

Comment 18

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6 - comments addressed

Carry forward r+ . I will see if I can get Stephen to look at this on Monday.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: Mockup by Stepehen

Stephen provided this awesome mockup. We now have to decide on one.
i particularly like the second, third and the last one from the top.

@Rob, Victor, any thoughts ?

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

I like numbers 13 (the last one) and 10 the most. The arrow icon gives an immediate hint on the nature of the event, whereas the other icons (info, warn, etc.) are more broad and would likely compel someone to switch over to the debugger tab to see what is going on.

I also find the orange background (not the top border highlight) too heavy and distracting, but the green and blue ones slightly less so.

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #20)
> I like numbers 13 (the last one) and 10 the most. The arrow icon gives an
> immediate hint on the nature of the event, whereas the other icons (info,
> warn, etc.) are more broad and would likely compel someone to switch over to
> the debugger tab to see what is going on.
> 
> I also find the orange background (not the top border highlight) too heavy
> and distracting, but the green and blue ones slightly less so.

Cool. I think showing two icons will be too much, so lets go with 13 ?

We can keep the green glow for general highlighting (as in any tab via the toolbox method) and use the special icon for Debugger's highlighted state. Anyways, any other tool is not using the highlighting api as of now. [They can have their own custom icon if they start using later on]

Comment 22

[Victor Porof [:vporof][:vp]]

I like the idea of putting the glow underneath. This respects the app tabs style/metaphor when having notifications and keeps things consistent. I also think that having two icons is a bit too much.

My vote goes for a combination of 13 and 9.

Comment 23

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #22)
> I like the idea of putting the glow underneath. This respects the app tabs
> style/metaphor when having notifications and keeps things consistent. I also
> think that having two icons is a bit too much.
> 
> My vote goes for a combination of 13 and 9.

9 --- Isn't that a little too much green. HULK MODE!!

Comment 24

[Victor Porof [:vporof][:vp]]

I was thinking more like 13 with the glow on the bottom.

Comment 25

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #24)
> I was thinking more like 13 with the glow on the bottom.

gotcha. so 13 with bottom glow.

Let's wait for Rob's input here and finalize then.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Initially I also preferred the glow underneath, but my argument against it is that it breaks the pretense of the glow being a reflection of the "active" content inside the tool panel.

I like thinking of the tab as a hollow structure that reflects light that comes from the open bottom border (since it is part of the same "page"), or even thinking of the main panel area as a room with the light turned on when I am visiting it. Putting the glow underneath breaks this mental model and reduces the glow to a generic ornament with an artificial connotation.

And that was the end of my foray into visual design for the day :-)

Comment 27

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #26)
> Initially I also preferred the glow underneath, but my argument against it
> is that it breaks the pretense of the glow being a reflection of the
> "active" content inside the tool panel.
> 
> I like thinking of the tab as a hollow structure that reflects light that
> comes from the open bottom border (since it is part of the same "page"), or
> even thinking of the main panel area as a room with the light turned on when
> I am visiting it. Putting the glow underneath breaks this mental model and
> reduces the glow to a generic ornament with an artificial connotation.
> 
> And that was the end of my foray into visual design for the day :-)

That's .. so deep, I can't even see you right now. But I really liked this perspective of the glow.

Comment 28

[Rob Campbell [:rc] (:robcee)]

I think we should render all our tabs as raytraces so they accurately reflect the shadows cast by the glows into the tab ... and beyond!

I'm with Victor and Panos. 13 with glow on the bottom.

(though really, those mockups are so sexy I'd be happy with any of them).

Do we have color values for those glows?

Comment 29

[Girish Sharma [:Optimizer]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #28)
> I think we should render all our tabs as raytraces so they accurately
> reflect the shadows cast by the glows into the tab ... and beyond!
> 
> I'm with Victor and Panos. 13 with glow on the bottom.

I thought Panos was with the glow on top ... now I am confused.

> (though really, those mockups are so sexy I'd be happy with any of them).
> 
> Do we have color values for those glows?

No, but they should be easy to ask once we are fixated on one thing.

Comment 30

[Girish Sharma [:Optimizer]]

Hi Stephen, Can we get the icon and CSS for the last highlighted state in the mockup ?

Comment 31

[Girish Sharma [:Optimizer]]

Attachment: patch v0.7 implementing the mockup

This implements the last style in the mockup. I had to use some weird margin and padding values as I had to display:none the image in the radio and give padding and background image to the label. This sometimes causes flicker when you switch b/w debugger and other tool when debugger is paused.

The other approach will be to drop the <radio>'s <image> all together and implement the icon via style.backgroundImage.

Ideas welcome if someone can think of any other approach.

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 758649 [details] [diff] [review]
patch v0.7 implementing the mockup

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

I'm largely ok with these changes but haven't viewed this new build yet. I'd kind of like paul's input on the style changes. Thanks for sticking with this, Optimizer!

::: browser/devtools/debugger/test/browser_dbg_bug868163_highight_on_pause.js
@@ +44,5 @@
> +      }).then(testResume);
> +    }}, 0);
> +  });
> +
> +  EventUtils.sendMouseEvent({ type: "mousedown" },

shoot. are we still using mousedown instead of click events for these?

@@ +47,5 @@
> +
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> +    gDebugger.document.getElementById("resume"),
> +    gDebugger);
> +}

for this test you're clicking the resume button and then switching to the console onPause.

Do we have the case where you're on the console or highlighter and the debugger gets paused? Probably worth testing that case as well.

::: browser/themes/linux/devtools/toolbox.css
@@ +233,5 @@
> +  background-image: radial-gradient(farthest-corner at center top, #c0ff40, hsla(80,100%,63%,.5) 70%, hsla(80,100%,63%,.3) 97%),
> +                    radial-gradient(farthest-side at center top, hsla(80,100%,35%,.5), hsla(80,100%,35%,0)),
> +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> +                    linear-gradient(hsla(99,100%,14%,.2), hsla(99,100%,14%,.2));

did you get these directly from teh shorlander bot?

Comment 33

[Girish Sharma [:Optimizer]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #32)
> Comment on attachment 758649 [details] [diff] [review]
> patch v0.7 implementing the mockup
> 
> Review of attachment 758649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm largely ok with these changes but haven't viewed this new build yet. I'd
> kind of like paul's input on the style changes. Thanks for sticking with
> this, Optimizer!
> 
> ::: browser/devtools/debugger/test/browser_dbg_bug868163_highight_on_pause.js
> @@ +44,5 @@
> > +      }).then(testResume);
> > +    }}, 0);
> > +  });
> > +
> > +  EventUtils.sendMouseEvent({ type: "mousedown" },
> 
> shoot. are we still using mousedown instead of click events for these?

I just replicated another of the debugger's test ;)

> @@ +47,5 @@
> > +
> > +  EventUtils.sendMouseEvent({ type: "mousedown" },
> > +    gDebugger.document.getElementById("resume"),
> > +    gDebugger);
> > +}
> 
> for this test you're clicking the resume button and then switching to the
> console onPause.
> 
> Do we have the case where you're on the console or highlighter and the
> debugger gets paused? Probably worth testing that case as well.

framework/test/browser_toolbox_highlight.js is the test that does kind of that. Should be enough I think.

> ::: browser/themes/linux/devtools/toolbox.css
> @@ +233,5 @@
> > +  background-image: radial-gradient(farthest-corner at center top, #c0ff40, hsla(80,100%,63%,.5) 70%, hsla(80,100%,63%,.3) 97%),
> > +                    radial-gradient(farthest-side at center top, hsla(80,100%,35%,.5), hsla(80,100%,35%,0)),
> > +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> > +                    linear-gradient(hsla(204,45%,98%,.05), hsla(204,45%,98%,.1)),
> > +                    linear-gradient(hsla(99,100%,14%,.2), hsla(99,100%,14%,.2));
> 
> did you get these directly from teh shorlander bot?

Yup.

Comment 34

[Girish Sharma [:Optimizer]]

Comment on attachment 758649 [details] [diff] [review]
patch v0.7 implementing the mockup

Clearing review for now as one witch told me a spell to remove the flickering completely. The spell needs preparations, so I will post the patch tonight.

Comment 36

[Girish Sharma [:Optimizer]]

Attachment: final patch

This does not flicker. I just want to make sure that the alignment is correct on Mac and Linux too.

Can anybody post images of the debugger tab for the three states on Linux and Mac:

1) Before the patch
2) After the patch, normal
3) After the patch, debugger on pause with different seelcted tool

Comment 37

[Paul Rouget [:paul]]

Attachment: Linux screenshots

Comment 38

[Paul Rouget [:paul]]

Attachment: Mac screenshots

Comment 39

[Paul Rouget [:paul]]

Comment on attachment 763077 [details] [diff] [review]
final patch

Setting the new icon at the CSS level doesn't sound like the best way to handle it, it looks like a hack (background-size: 0px 16px).

You introduce a new JS API to highlight a tool. That's good. Why not introduce a new property in the tool definition? `icon_highlight: "url"`. Code will be more simple, and we won't have debugger-specific CSS code. And this will ensure that the 2 icons will be rendered the same way.

Also - don't we want to keep the icon green even when the tab is selected? It's kind of confusing when we switch to a new tool, it turns green, and then you ask yourself "something happened?", and you switch back to the debugger to understand what happened, and then it turns back to blue.

Comment 40

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #39)
> Comment on attachment 763077 [details] [diff] [review]
> final patch
> 
> Setting the new icon at the CSS level doesn't sound like the best way to
> handle it, it looks like a hack (background-size: 0px 16px).
> 
> You introduce a new JS API to highlight a tool. That's good. Why not
> introduce a new property in the tool definition? `icon_highlight: "url"`.
> Code will be more simple, and we won't have debugger-specific CSS code. And
> this will ensure that the 2 icons will be rendered the same way.

Agreed. Done locally.

> Also - don't we want to keep the icon green even when the tab is selected?
> It's kind of confusing when we switch to a new tool, it turns green, and
> then you ask yourself "something happened?", and you switch back to the
> debugger to understand what happened, and then it turns back to blue.

The purpose of highlight APi was to let the user know that a tool wants your attention. Thus the highlighted color was removed when the tool was selected. It got your attention, so the purpose of highlighting was fulfilled. But lets ask experts on this idea of not removing highlight. Stephen, any thoughts ?

Comment 41

[Girish Sharma [:Optimizer]]

Attachment: final patch part 2

Using JS to change the icon. Added a new option in the tool definition named "highlightedicon".

Update the tests to check that the icon is correctly switched.

Comment 42

[Paul Rouget [:paul]]

Comment on attachment 764837 [details] [diff] [review]
final patch part 2

s/dehighlight/unhighlight/

The logic "show highlighted icon only if not selected" is build in JS. Could we do that in CSS instead?

A bit like you did in the first patch, but in a generic way.

For example, we could have 2 image tags, one with the non-highlighted icon, one with the highlighted icon (which would be the non-highlighted icon if there's no icon specified in the definition), and we would only show the second one if the tab is highlighted and not selected.

In JS, we define the highlighted icon URL, in CSS we choose which icon to show.

What do you think?

Comment 43

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #42)
> Comment on attachment 764837 [details] [diff] [review]
> final patch part 2
> 
> s/dehighlight/unhighlight/
> 
> The logic "show highlighted icon only if not selected" is build in JS. Could
> we do that in CSS instead?
> 
> A bit like you did in the first patch, but in a generic way.
> 
> For example, we could have 2 image tags, one with the non-highlighted icon,
> one with the highlighted icon (which would be the non-highlighted icon if
> there's no icon specified in the definition), and we would only show the
> second one if the tab is highlighted and not selected.
> 
> In JS, we define the highlighted icon URL, in CSS we choose which icon to
> show.
> 
> What do you think?

I think it would be too overboard: Creating two icons, overlapping with negative margins, opacity: 0 on one of them at a time, making sure it works well on all OS.
In JS, I only change an attribute (src), which is pretty light weight, if that is the concern. Although, I have to traverse the whole tablist.

Adding overlapping two images and only showing one at a time would be kind of hacky in my opinion.

Comment 44

[Paul Rouget [:paul]]

Just display:none the icon that is not used.

Comment 45

[Girish Sharma [:Optimizer]]

I tried the approach, I can see the flickering just like the one I saw in comment 31. I want to avoid flickering as it causes the whole tabbar to reflow and repaint. Not to mention the juggling label.

Comment 46

[Girish Sharma [:Optimizer]]

Attachment: super final patch

- Changed the name to unhighlightTool
- Using CSS to switch icon's visibility
- does not flicker as using visibility:collapse (thanks Paul for the tip)

(reverted back the changes that I did for converting icon shift from cSS to JS)

Comment 47

[Paul Rouget [:paul]]

Comment on attachment 765992 [details] [diff] [review]
super final patch

r+ with s/normal/default/

Comment 48

[Girish Sharma [:Optimizer]]

Attachment: super final patch using "default-icon"

Ship it!

Comment 49

[Girish Sharma [:Optimizer]]

Attachment: rebased on latest fx-team

Rebased on latest fx-team. It would be awesome if someone could land it so that it gets in 24

Comment 50

[Girish Sharma [:Optimizer]]

Attachment: rebased once again

Comment 51

[Paul Rouget [:paul]]

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

Comment 52

[Victor Porof [:vporof][:vp]]

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