1049020 - DevTools Themes: Update TableWidget headers to match new theme

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Albert Juhé]

Coming from bug 951714 comment 60. We have to update the TableWidget headers to match the new theme as we did for the network headers.

Comment 1

[Girish Sharma [:Optimizer]]

Thanks :)

Comment 3

[Tim Nguyen :ntim]

Attachment: WIP

I probably won't work on this, but I've been tackling this quite recently, so here it is.

Albert : I would recommend you to start with this bug first rather than starting with the network monitor, since you don't have to think about the width, or the timestamps.

Comment 4

[Girish Sharma [:Optimizer]]

Comment on attachment 8485454 [details] [diff] [review]
WIP

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

::: browser/devtools/shared/widgets/TableWidget.js
@@ +468,5 @@
>    this.column.id = id;
>    this.column.className = "table-widget-column";
>    this.wrapper.appendChild(this.column);
>  
> +  this.header = this.document.createElementNS(HTML_NS, "label");

This should not be changed.

Comment 5

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #4)
> Comment on attachment 8485454 [details] [diff] [review]
> WIP
> 
> Review of attachment 8485454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/TableWidget.js
> @@ +468,5 @@
> >    this.column.id = id;
> >    this.column.className = "table-widget-column";
> >    this.wrapper.appendChild(this.column);
> >  
> > +  this.header = this.document.createElementNS(HTML_NS, "label");
> 
> This should not be changed.

That's the only way to add the sort arrow.

Comment 6

[Girish Sharma [:Optimizer]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Girish Sharma [:Optimizer] from comment #4)
> > Comment on attachment 8485454 [details] [diff] [review]
> > WIP
> > 
> > Review of attachment 8485454 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/shared/widgets/TableWidget.js
> > @@ +468,5 @@
> > >    this.column.id = id;
> > >    this.column.className = "table-widget-column";
> > >    this.wrapper.appendChild(this.column);
> > >  
> > > +  this.header = this.document.createElementNS(HTML_NS, "label");
> > 
> > This should not be changed.
> 
> That's the only way to add the sort arrow.

I see. XUL :(

Comment 7

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > (In reply to Girish Sharma [:Optimizer] from comment #4)
> > > Comment on attachment 8485454 [details] [diff] [review]
> > > WIP
> > > 
> > > Review of attachment 8485454 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: browser/devtools/shared/widgets/TableWidget.js
> > > @@ +468,5 @@
> > > >    this.column.id = id;
> > > >    this.column.className = "table-widget-column";
> > > >    this.wrapper.appendChild(this.column);
> > > >  
> > > > +  this.header = this.document.createElementNS(HTML_NS, "label");
> > > 
> > > This should not be changed.
> > 
> > That's the only way to add the sort arrow.
> 
> I see. XUL :(

Not really, it's just that the way label is used makes label a replaced element. And pseudo elements don't work on replaced elements (that's according to the spec).

In XUL, the value attribute forces the label to be a replaced element. If you remove the value attribute and set the textContent instead, the label is no longer a replaced element. I didn't really know how to fix this, so I just put a temporary workaround. I think it'd be better to file a bug for this.

Comment 8

[Girish Sharma [:Optimizer]]

I see, then you can always do .textContent instead of setting value attribute on the label itself and it works.

Comment 9

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> I see, then you can always do .textContent instead of setting value
> attribute on the label itself and it works.

But it breaks the header context menu.

Comment 10

[Girish Sharma [:Optimizer]]

I removed the value attribute and added textContent. The text was around 2px below from its original position, but apart from that, everything was working, clicking/context menu etc. Are you sure context menu is broken for you ?

Comment 11

[Tim Nguyen :ntim]

Attachment: Patch

Stealing this from Albert, hope you don't mind :)

This changes the sort arrow SVGs, so the SVG dimensions match the arrow size in the netmonitor (it was previously stretched out). Now it still looks the same, but the arrow isn't stretched out anymore.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8697886 [details] [diff] [review]
Patch

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

Looks good!  Can we somehow share this styling through a set of classes, or is the markup too different?

Comment 13

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8697886 [details] [diff] [review]
> Patch
> 
> Review of attachment 8697886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!  Can we somehow share this styling through a set of classes, or
> is the markup too different?

TableWidget's markup is column based, while the netmonitor one is row based, so sharing the styling will be quite tricky.

Comment 14

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/335f67b728a4

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8697886 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 951714
[User impact if declined]: inconsistent look between netmonitor and storage inspector tables
[Describe test coverage new/current, TreeHerder]: on m-c, tested locally
[Risks and why]: low, css only change
[String/UUID change made/needed]: none

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8697886 [details] [diff] [review]
Patch

Polish, should be safe, taking it.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8db8db2ac83d

Comment 19

[Mayur Patil]

[bugday-20160323]

Though it is developer specific testing bug but clear STR helps to verify during testdays.

Status: RESOLVED,FIXED --> UNVERIFIED

Component: 
Name 	        Firefox
Version 	46.0b2
Build ID 	20160314144540
Update Channel 	beta
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS              Windows 7 SP1 x86_64