665907 - [highlighter] Bounding boxes update are slow while the page is scrolling.

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Paul Rouget [:paul]]

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)]

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 3

[Mihai Sucan [:msucan]]

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)]

Could probably just disable transitions once the node is locked, and enable them again when unlocked.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: 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]]

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]]

Attachment: updated patch

Updated the patch.

Comment 8

[Mihai Sucan [:msucan]]

Dão: btw, thanks a lot for your really quick response to the patch!

Comment 9

[Dão Gottwald [:dao]]

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]]

(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]]

I think at the point where the JS gets messy, you should use the child selector.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: [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)]

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]]

Thank you Robert and Dão!

This patch can now land in fx-team.

Comment 15

[Rob Campbell [:rc] (:robcee)]

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] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/cf5fb6035476