Closed Bug 884432 Opened 7 years ago Closed 7 years ago

Background color of request items keep changing when any filter is applied

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: Optimizer, Assigned: vporof)

Details

Attachments

(1 file, 1 obsolete file)

Go to any site like this page.

Filter on something like JS

Hit Ctrl F5

See the rows do disco!
Yeah, it's because of CSS and nth-child.
Severity: normal → enhancement
Priority: -- → P3
OS: Windows 7 → All
Hardware: x86_64 → All
(In reply to Victor Porof [:vp] from comment #1)
> Yeah, it's because of CSS and nth-child.

Can the reordering in case of any kind of sorting be done using ordinal property and not shifting the elements around. Maybe that will make the nth-child work as is even if the element is shifted ?
(In reply to Girish Sharma [:Optimizer] from comment #2)
> (In reply to Victor Porof [:vp] from comment #1)
> > Yeah, it's because of CSS and nth-child.
> 
> Can the reordering in case of any kind of sorting be done using ordinal
> property and not shifting the elements around. Maybe that will make the
> nth-child work as is even if the element is shifted ?

I doubt it.

Also, sorting has nothing to do with filtering. The background color change is an artifact of "nth child" not meaning "nth visible child".

We should just set the odd/even classes in js, it's not that big of a deal.
So, here's the rationale why it has to do with sorting && filtering.

If the rows are sorted on timeline, that is the default scenario, then any new row added will be added to the end and the index of already existing rows will not change. Now however you filter, then the row's background will remain same.

But if we have sorted the rows on Name, or anything else, which needs insertion in b/w existing rows while adding a new row, that will change the index of rows after the newly added row, thus changing their odd/even property.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Needs a test.
Attached patch v1.1Splinter Review
Added test.
Attachment #765296 - Attachment is obsolete: true
Attachment #765308 - Flags: review?(rcampbell)
Comment on attachment 765308 [details] [diff] [review]
v1.1

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

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +489,5 @@
>          break;
>      }
> +
> +    this.refreshSummary();
> +    this.refreshZebra();

could almost bundle refreshZebra into refreshSummary, but that might cause other problems.

@@ +608,5 @@
> +   */
> +  refreshZebra: function() {
> +    let visibleItems = this.orderedVisibleItems;
> +
> +    for (let i = 0, len = visibleItems.length; i < len; i++) {

what? No forEach? No clever array comprehension? No map()? I'm...

are you feeling ok?

@@ +614,5 @@
> +      let requestTarget = requestItem.target;
> +
> +      if (i % 2 == 0) {
> +        requestTarget.setAttribute("even", "");
> +        requestTarget.removeAttribute("odd");

I feel like these should be classes. Why the attribute?

::: browser/themes/linux/devtools/netmonitor.css
@@ +311,5 @@
>  }
>  
>  /* SideMenuWidget */
>  
> +.side-menu-widget-item[odd] {

an odd class would work here wouldn't it?
Attachment #765308 - Flags: review?(rcampbell)
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> 
> ::: browser/devtools/netmonitor/netmonitor-view.js
> @@ +489,5 @@
> >          break;
> >      }
> > +
> > +    this.refreshSummary();
> > +    this.refreshZebra();
> 
> could almost bundle refreshZebra into refreshSummary, but that might cause
> other problems.
> 

Those handle completely different things, so I'd vouch for.. no? RefreshSummary() refreshes the "x request in y time" notification at the bottom, so it doesn't really make sense to also change the background classes on each item in the same function.

> @@ +608,5 @@
> > +   */
> > +  refreshZebra: function() {
> > +    let visibleItems = this.orderedVisibleItems;
> > +
> > +    for (let i = 0, len = visibleItems.length; i < len; i++) {
> 
> what? No forEach? No clever array comprehension? No map()? I'm...
> 
> are you feeling ok?

Depression is kicking in. Nothing's worth it anymore.

> 
> @@ +614,5 @@
> > +      let requestTarget = requestItem.target;
> > +
> > +      if (i % 2 == 0) {
> > +        requestTarget.setAttribute("even", "");
> > +        requestTarget.removeAttribute("odd");
> 
> I feel like these should be classes. Why the attribute?
> 

Using classList may automatically cause reflows, especially adding classes, but I'm not entirely sure. It felt like attributes may be safer.

> ::: browser/themes/linux/devtools/netmonitor.css
> @@ +311,5 @@
> >  }
> >  
> >  /* SideMenuWidget */
> >  
> > +.side-menu-widget-item[odd] {
> 
> an odd class would work here wouldn't it?

I was also confused about this first, but no :) CSS nth-child start at index 1, but I'm counting from 0 like real programmers do.
Attachment #765308 - Flags: review?(rcampbell)
(In reply to Victor Porof [:vp] from comment #8)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> > 
> > ::: browser/devtools/netmonitor/netmonitor-view.js
> > @@ +489,5 @@
> > >          break;
> > >      }
> > > +
> > > +    this.refreshSummary();
> > > +    this.refreshZebra();
> > 
> > could almost bundle refreshZebra into refreshSummary, but that might cause
> > other problems.
> 
> Those handle completely different things, so I'd vouch for.. no?
> RefreshSummary() refreshes the "x request in y time" notification at the
> bottom, so it doesn't really make sense to also change the background
> classes on each item in the same function.

Right.

> > > +    for (let i = 0, len = visibleItems.length; i < len; i++) {
> > 
> > what? No forEach? No clever array comprehension? No map()? I'm...
> > 
> > are you feeling ok?
> 
> Depression is kicking in. Nothing's worth it anymore.

AW!

> > >  /* SideMenuWidget */
> > >  
> > > +.side-menu-widget-item[odd] {
> > 
> > an odd class would work here wouldn't it?
> 
> I was also confused about this first, but no :) CSS nth-child start at index
> 1, but I'm counting from 0 like real programmers do.

OK.
Attachment #765308 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/edfa5974a68e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.