My favorites | Sign in
Project Home Wiki Issues
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 5850: Modifying CSS doesn't update element visibility in HTML panel
1 person starred this issue and may be notified of changes. Back to list
 
Project Member Reported by simon.lindholm10, Aug 21, 2012
What steps will reproduce the problem?
1. Right-click on this text, choose "Inspect Element With Firebug".
2. Through the Style panel, add "display: none;" to some rule affecting it.

What is the expected output? What do you see instead?
The element should be marked as invisible in the HTML panel, but it isn't.

Aug 21, 2012
Project Member #1 simon.lindholm10
(No comment was entered for this change.)
Blocking: fbug:5848
Aug 21, 2012
Project Member #2 sebastia...@gmail.com
Simon, could that be the same as  issue 1690 ?

Sebastian
Cc: sebastia...@gmail.com
Aug 21, 2012
Project Member #3 simon.lindholm10
No. That issue is for "disabled" attributes breaking mutation events (a platform bug); this is just about hooking up CSS rule modification to the HTML panel.
Labels: style
Aug 21, 2012
Project Member #4 simon.lindholm10
Something quite like this.
5850.patch
3.9 KB   View   Download
Aug 23, 2012
Project Member #5 sebastia...@gmail.com
The patch already looks good. Though there are still two issues:
- The visibility is not updated when removing the rule. A little investigation showed that Xml.isVisible() still returns false for them.
- The visibility is not just influenced by the "display" property. Xml.isVisible() determines it by checking the offsetWidth and offsetHeight among other things. So an empty element, which has it's width set to 0 should also be marked as invisible.

Sebastian
Status: Started
Owner: simon.lindholm10
Aug 24, 2012
Project Member #6 simon.lindholm10
> - The visibility is not updated when removing the rule.
Ah, good catch. Should we do this.context.setTimeout(, 0) or introduce an "onAfterCSSDeleteRule"?

> - The visibility is not just influenced by the "display" property. Xml.isVisible()
> determines it by checking the offsetWidth and offsetHeight among other things. So an
> empty element, which has it's width set to 0 should also be marked as invisible.
I know - my current WIP contains the comment "For now, pretend that "display" is the only property which affects visibility.". I do want to keep the optimization, but then there are two options: Adding checks also for (min-,max-) height, width, borders, margin, padding, etc., or changing the definition of visibility (note that having offsetHeight = offsetWidth = 0 doesn't necessarily mean you're invisible). Not sure what I prefer. Thoughts?
Aug 25, 2012
Project Member #7 sebastia...@gmail.com
> Ah, good catch. Should we do this.context.setTimeout(, 0) or introduce an "onAfterCSSDeleteRule"?
The second one sounds cleaner.

> Not sure what I prefer. Thoughts?
I'd prefer to remove the optimization for now and add a hint that there's space for optimization. :-)

Sebastian
Sep 1, 2012
Project Member #8 simon.lindholm10
> The second one sounds cleaner.
Okay, did that.

> I'd prefer to remove the optimization for now and add a hint that there's space
> for optimization. :-)
I profiled some, and it's clear that we really do need the optimization. E.g., when I open the single-page HTML5 spec (or rather, the first third, because Firefox runs out of RAM and crashes if I don't cancel the load midway through), inspect a link, and change a CSS property of the ":link" rule, the browser freezes for _5 seconds_ trying to run through all the links on the page and query offsetWidth/Height. (Normally, the reflows etc. from CSS changes etc. are async and take ~0.5 seconds.)
In more realistic cases, it still adds many milliseconds to a property change. Not really okay for fixing a minor bug. :/

Delays on changing "display" should be more okay - it probably won't be set on a lot of elements (mainly block containers and such), and likely actually affects visibility statuses.

Do you think this looks good? https://github.com/simonlindholm/firebug/commit/acbb65848765d9bbf914bcfa723c1d143017ab73
Sep 14, 2012
Project Member #10 simon.lindholm10
Eh, I'll just commit it. (It looks good to me at least.)

https://github.com/firebug/firebug/commit/e4c3e0ef54b1361d0ed1e1bdbf73343cb46835d2
Status: Commit
Sep 15, 2012
Project Member #11 sebastia...@gmail.com
Sorry for not replying earlier, Simon!

Yes, your patch seems ok to me.
We should keep the limitations in mind, though. (I.e. create a new issue for them.)

> Normally, the reflows etc. from CSS changes etc. are async and take ~0.5 seconds.
In an optimized HTML panel we would do all display updates asynchronously and avoid blocking the UI.

Sebastian
Labels: FBTest-wanted
Oct 5, 2012
Project Member #12 odva...@gmail.com
This issue has been fixed in Firebug 1.11a4
https://getfirebug.com/releases/firebug/1.11/firebug-1.11.0a4.xpi

Please try it and let us know if it works for you

Thanks for the help!
Honza
Status: Fixed
Labels: fixed-1.11-a4
Jun 10, 2013
Project Member #13 simon.lindholm10
> In an optimized HTML panel we would do all display updates asynchronously and avoid blocking the UI.
Reflow is synchronous when triggered with JavaScript, so that only helps so much. Only checking the visible elements would be faster though.

https://bugzilla.mozilla.org/show_bug.cgi?id=878801 could help slightly.
Sign in to add a comment

Powered by Google Project Hosting