Closed Bug 665907 Opened 10 years ago Closed 10 years ago

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

Categories

(DevTools :: General, defect, P2)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: paul, Assigned: msucan)

References

Details

(Keywords: polish, Whiteboard: [minotaur][best: 1h; likely: 4h; worst: 2d])

Attachments

(1 file, 2 obsolete files)

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.
Summary: [highlighter] Bounding boxes are slow while the page is scrolling. → [highlighter] Bounding boxes update are slow while the page is scrolling.
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?
Whiteboard: [minotaur]
Keywords: polish
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][best: 1h; likely: 4h; worst: 2d]
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Duplicate of this bug: 676175
Assignee: rcampbell → mihai.sucan
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?
Could probably just disable transitions once the node is locked, and enable them again when unlocked.
Priority: P1 → P2
Attached patch proposed patch (obsolete) — Splinter Review
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!
Attachment #556860 - Flags: review?(rcampbell)
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).
Attached patch updated patch (obsolete) — Splinter Review
Updated the patch.
Attachment #556860 - Attachment is obsolete: true
Attachment #556931 - Flags: review?(rcampbell)
Attachment #556931 - Flags: review?(dao)
Attachment #556860 - Flags: review?(rcampbell)
Dão: btw, thanks a lot for your really quick response to the patch!
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.
(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?
I think at the point where the JS gets messy, you should use the child selector.
Attachment #556931 - Flags: review?(dao)
Updated to use the child selector (from veil container to the nodes of interest), as suggested by Dão. Thank you!
Attachment #556931 - Attachment is obsolete: true
Attachment #557532 - Flags: review?(rcampbell)
Attachment #556931 - Flags: review?(rcampbell)
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+!
Attachment #557532 - Flags: review?(rcampbell) → review+
Thank you Robert and Dão!

This patch can now land in fx-team.
Whiteboard: [minotaur][best: 1h; likely: 4h; worst: 2d] → [minotaur][best: 1h; likely: 4h; worst: 2d][land-in-fx-team]
Comment on attachment 557532 [details] [diff] [review]
[in-fx-team] patch update 3

http://hg.mozilla.org/integration/fx-team/rev/cf5fb6035476
Attachment #557532 - Attachment description: patch update 3 → [in-fx-team] patch update 3
Whiteboard: [minotaur][best: 1h; likely: 4h; worst: 2d][land-in-fx-team] → [minotaur][best: 1h; likely: 4h; worst: 2d][fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/cf5fb6035476
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][best: 1h; likely: 4h; worst: 2d][fixed-in-fx-team] → [minotaur][best: 1h; likely: 4h; worst: 2d]
Target Milestone: --- → Firefox 9
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.