Closed Bug 1049020 Opened 10 years ago Closed 8 years ago

DevTools Themes: Update TableWidget headers to match new theme

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox45 fixed, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: me, Assigned: ntim, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → aljullu
Blocks: 916766
Status: NEW → ASSIGNED
Thanks :)
Attached patch WIP (obsolete) — Splinter Review
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 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.
(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.
(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 :(
(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.
I see, then you can always do .textContent instead of setting value attribute on the label itself and it works.
(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.
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 ?
Attached patch PatchSplinter Review
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.
Assignee: albertjuhe → ntim.bugs
Attachment #8485454 - Attachment is obsolete: true
Attachment #8697886 - Flags: review?(bgrinstead)
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?
Attachment #8697886 - Flags: review?(bgrinstead) → review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/335f67b728a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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
Attachment #8697886 - Flags: approval-mozilla-aurora?
Comment on attachment 8697886 [details] [diff] [review]
Patch

Polish, should be safe, taking it.
Attachment #8697886 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: