Last Comment Bug 665907 - [highlighter] Bounding boxes update are slow while the page is scrolling.
: [highlighter] Bounding boxes update are slow while the page is scrolling.
Status: RESOLVED FIXED
[minotaur][best: 1h; likely: 4h; wors...
: polish
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Linux
: P2 normal (vote)
: Firefox 9
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 676175 (view as bug list)
Depends on:
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-06-21 08:24 PDT by Paul Rouget [:paul]
Modified: 2011-09-08 02:42 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (4.24 KB, patch)
2011-08-30 08:49 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
updated patch (6.35 KB, patch)
2011-08-30 11:26 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
[in-fx-team] patch update 3 (6.37 KB, patch)
2011-09-01 09:35 PDT, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-06-21 08:24:08 PDT
Drawing the veil and/or the layout boxes is slow (we can see the boxes "catching up" the page).

A solution could be to display:none these boxes while scrolling.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-06-21 10:59:51 PDT
I noticed this too. display:none could be a solution. I'm also wondering if it's related to the transition. Maybe setting the transition times to 0 for scroll operations would help this?
Comment 2 Rob Campbell [:rc] (:robcee) 2011-08-03 11:33:06 PDT
*** Bug 676175 has been marked as a duplicate of this bug. ***
Comment 3 Mihai Sucan [:msucan] 2011-08-29 09:47:45 PDT
The apparent slowness is caused by the animation of the veil. Instead of display:none, I'd suggest setting transition times to 0, like Rob also suggested. Agreed?

What events shall we use to catch before scroll and after scroll?
Comment 4 Dave Camp (:dcamp) 2011-08-29 09:51:17 PDT
Could probably just disable transitions once the node is locked, and enable them again when unlocked.
Comment 5 Mihai Sucan [:msucan] 2011-08-30 08:49:05 PDT
Created attachment 556860 [details] [diff] [review]
proposed patch

Proposed patch. As suggested by Dave, animations are now disabled once the user locks on an element.

Please let me know if this is fine. Should we have a test for this? I saw there was no test for bug 669656 either.

Shall I ask Dão for review as well?

Thank you!
Comment 6 Dão Gottwald [:dao] 2011-08-30 08:52:08 PDT
Comment on attachment 556860 [details] [diff] [review]
proposed patch

>--- a/browser/base/content/highlighter.css
>+++ b/browser/base/content/highlighter.css

>-.highlighter-veil,
>-#highlighter-veil-middlebox,
>-#highlighter-veil-transparentbox {
>+#highlighter-veil-container:not([locked]) .highlighter-veil,
>+#highlighter-veil-container:not([locked]) #highlighter-veil-middlebox,
>+#highlighter-veil-container:not([locked]) #highlighter-veil-transparentbox {

Please avoid the descendant selector (https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector).
Comment 7 Mihai Sucan [:msucan] 2011-08-30 11:26:13 PDT
Created attachment 556931 [details] [diff] [review]
updated patch

Updated the patch.
Comment 8 Mihai Sucan [:msucan] 2011-08-30 11:30:35 PDT
Dão: btw, thanks a lot for your really quick response to the patch!
Comment 9 Dão Gottwald [:dao] 2011-08-30 12:12:37 PDT
Comment on attachment 556931 [details] [diff] [review]
updated patch

>-.highlighter-veil,
>-#highlighter-veil-middlebox,
>-#highlighter-veil-transparentbox {
>+.highlighter-veil:not([locked]),
>+#highlighter-veil-middlebox:not([locked]),
>+#highlighter-veil-transparentbox:not([locked]) {

Instead of setting the attribute on numerous nodes, you could probably have used the child selector to get from #highlighter-veil-container to these nodes.
Comment 10 Mihai Sucan [:msucan] 2011-08-30 12:51:30 PDT
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 556931 [details] [diff] [review]
> updated patch
> 
> >-.highlighter-veil,
> >-#highlighter-veil-middlebox,
> >-#highlighter-veil-transparentbox {
> >+.highlighter-veil:not([locked]),
> >+#highlighter-veil-middlebox:not([locked]),
> >+#highlighter-veil-transparentbox:not([locked]) {
> 
> Instead of setting the attribute on numerous nodes, you could probably have
> used the child selector to get from #highlighter-veil-container to these
> nodes.

Dão: I wanted to use the child selector ( a > b ) but I wasn't sure that's acceptable, the wiki page suggests to be as direct as possible (when using selectors). Shall I use the child selector?
Comment 11 Dão Gottwald [:dao] 2011-08-30 13:06:51 PDT
I think at the point where the JS gets messy, you should use the child selector.
Comment 12 Mihai Sucan [:msucan] 2011-09-01 09:35:59 PDT
Created attachment 557532 [details] [diff] [review]
[in-fx-team] patch update 3

Updated to use the child selector (from veil container to the nodes of interest), as suggested by Dão. Thank you!
Comment 13 Rob Campbell [:rc] (:robcee) 2011-09-01 17:59:40 PDT
Comment on attachment 557532 [details] [diff] [review]
[in-fx-team] patch update 3

you already worked this out with Dao and the implementation looks decent. I checked the previous comments to figure out why you're creating a new vbox for the veil and understand why you're using a child selector.

easy r+!
Comment 14 Mihai Sucan [:msucan] 2011-09-02 09:04:35 PDT
Thank you Robert and Dão!

This patch can now land in fx-team.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-09-02 11:29:15 PDT
Comment on attachment 557532 [details] [diff] [review]
[in-fx-team] patch update 3

http://hg.mozilla.org/integration/fx-team/rev/cf5fb6035476
Comment 16 Tim Taubert [:ttaubert] 2011-09-08 02:42:24 PDT
http://hg.mozilla.org/mozilla-central/rev/cf5fb6035476

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