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

RESOLVED FIXED in Firefox 9

Status

P2
normal
RESOLVED FIXED
8 years ago
8 months ago

People

(Reporter: paul, Assigned: msucan)

Tracking

({polish})

unspecified
Firefox 9
x86
Linux
polish

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
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?

Updated

8 years ago
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)

Updated

8 years ago
Assignee: rcampbell → mihai.sucan
(Assignee)

Comment 3

8 years ago
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.

Updated

8 years ago
Priority: P1 → P2
(Assignee)

Comment 5

8 years ago
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!
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).
(Assignee)

Comment 7

8 years ago
Created attachment 556931 [details] [diff] [review]
updated patch

Updated the patch.
Attachment #556860 - Attachment is obsolete: true
Attachment #556931 - Flags: review?(rcampbell)
Attachment #556931 - Flags: review?(dao)
Attachment #556860 - Flags: review?(rcampbell)
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Comment 10

8 years ago
(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)
(Assignee)

Comment 12

8 years ago
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!
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+
(Assignee)

Comment 14

8 years ago
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
Last Resolved: 8 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

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.