Bug 84242 - Web Inspector: Radio buttons in Audits tabs don't work
: Web Inspector: Radio buttons in Audits tabs don't work
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-18 08:01 PST by
Modified: 2012-12-29 00:04 PST (History)


Attachments


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-18 08:01:18 PST
What steps will reproduce the problem?
1. Open any webpage in Chrome. (eg www.google.com).
2. Right click anywhere on it and select Inspect element.
3. Now click the Audits tab. Select the radio button "Reload Page and Audit on load"
[if the radiobuttons work for you, follow the next steps WITHOUT switching to another window (i.e. the entire step sequence must be run with the browser window always focused)]
4. Switch to the Profiles panel and back to the Audits window
5. Select the "Audits" tree element in the lefthand tree to open the audits launcher view. Now the radio buttons look dead.

What is the expected result?
The radio button should toggle the option we click another one.

What happens instead?
The radio button don't toggle.

(Upstreaming http://code.google.com/p/chromium/issues/detail?id=123289)

Now some investigation results:
1. The radio buttons do toggle but are never repainted. This can be checked by clicking "Audit Present State" and "Run" - the audits will run without reloading the page.
2. When steps 4 and 5 are run (I only very rarely can reproduce the issue in Chromium without them), certain container DIVs corresponding to removed views are detached from the DOM tree to speed up rendering, and those corresponding to shown panels are inserted (element.appendChild) - this is the common way to switch views in Web Inspector.
3. When a radio button is clicked, HTMLInputElement::setChecked() invokes setNeedsStyleRecalc(), which calls markAncestorsWithChildNeedsStyleRecalc(). Its loop stops quickly (like, at the parent <label> element, for which childNeedsStyleRecalc() == true), and I believe the algorithm presumes that this condition holds for all its ancestors. However, this seems wrong: if I comment out the "&& !p->childNeedsStyleRecalc()" part of the condition, the radio buttons start working as expected. Stepping through the loop (with this fix in place), I found out that p->childNeedsStyleRecalc() == false for the container DIV that was detached (element.removeChild) and then re-inserted (element.appendChild) into the Web Inspector DOM, as well as its ancestors chain.
4. I thought that it would be sufficient to add
  setChildNeedsStyleRecalc();
  markAncestorsWithChildNeedsStyleRecalc();
at the end of Node::attach() but it proved wrong - I observed the absence of attached view contents altogether.
5. I noticed that lazyAttach() runs markAncestorsWithChildNeedsStyleRecalc() but could not deduce what lazyAttach() is for, and when it comes into play instead of attach() (I saw some work by jamesr@ to retain lazyAttach() as the only option.)

Does anyone have any insight of what's happening/how this works and what can be done to fix this? (Obviously, not the "Web Inspector" component...)
------- Comment #1 From 2012-04-18 08:14:07 PST -------
Do you have a regression range?
------- Comment #2 From 2012-04-18 08:20:14 PST -------
(In reply to comment #1)
> Do you have a regression range?

Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...
------- Comment #3 From 2012-04-18 08:21:46 PST -------
(In reply to comment #2)
> (In reply to comment #1)
> > Do you have a regression range?
> 
> Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...

Hmm, I seem to have come up with a reduced test case, so will try to bisect now.
------- Comment #4 From 2012-04-19 05:56:38 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Do you have a regression range?
> > 
> > Unfortunately, not. While bisecting, I ended up with my own change that introduced the updated DOM layout which manifested the bug (around November 2011), which means the bug has been sitting there for a while...
> 
> Hmm, I seem to have come up with a reduced test case, so will try to bisect now.

Got it. http://trac.webkit.org/changeset/83065

Addendum: Step 3 above MUST be followed by "Click the Run button", obviously.

I've got (an invisible) progress indicator to the right of the "Run" button, if that matters (and that should.) It shows up when the "Run" button is clicked, and gets hidden when the page has reloaded and audits have finished running.
------- Comment #5 From 2012-04-19 07:11:34 PST -------
(In reply to comment #4)
> > 
> > Hmm, I seem to have come up with a reduced test case, so will try to bisect now.
> 
> Got it. http://trac.webkit.org/changeset/83065
> 
> Addendum: Step 3 above MUST be followed by "Click the Run button", obviously.
> 
> I've got (an invisible) progress indicator to the right of the "Run" button, if that matters (and that should.) It shows up when the "Run" button is clicked, and gets hidden when the page has reloaded and audits have finished running.

Hmm interesting. I'm interesting. Since I'm in travel (for the meeting),
I cannot take deeper look in a week. But I'll do after going back.
------- Comment #6 From 2012-06-05 01:11:05 PST -------
Presumably, a stackoverflow user is hitting a manifestation of the same bug: http://stackoverflow.com/questions/10892142/progress-bar-element-breaking-chrome-rendering-unless-dev-tools-open.

Hajime, could you take a look? The issue is easily reproducible in the DevTools.
------- Comment #7 From 2012-06-08 09:01:37 PST -------
Seemingly, the issue gets fixed by rewriting didElementStateChange() as follows:

void HTMLProgressElement::didElementStateChange()
{
    m_value->setWidthPercentage(position() * 100);
    if (renderer() && renderer()->isProgress()) {
        renderer()->updateFromElement();
        setNeedsStyleRecalc();
    }
}

Hajime, can you tell if this is the right way to fix this?
------- Comment #8 From 2012-06-14 01:53:21 PST -------
(In reply to comment #7)
> Seemingly, the issue gets fixed by rewriting didElementStateChange() as follows:
> 
> void HTMLProgressElement::didElementStateChange()
> {
>     m_value->setWidthPercentage(position() * 100);
>     if (renderer() && renderer()->isProgress()) {
>         renderer()->updateFromElement();
>         setNeedsStyleRecalc();
>     }
> }
> 
> Hajime, can you tell if this is the right way to fix this?
Sorry for the slow response. Looks like a safe change.
http://code.google.com/p/chromium/issues/detail?id=132014 also indicates
we miss some style recalculation request.
------- Comment #9 From 2012-06-18 09:25:52 PST -------
> > Hajime, can you tell if this is the right way to fix this?
> Sorry for the slow response. Looks like a safe change.
> http://code.google.com/p/chromium/issues/detail?id=132014 also indicates
> we miss some style recalculation request.

Well, this did not fix http://code.google.com/p/chromium/issues/detail?id=132014 for me, so there's something else getting in the way...
------- Comment #10 From 2012-12-29 00:04:09 PST -------
Looks fixed as of r138555.