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

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Developer Tools
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: msucan)

Tracking

({polish})

unspecified
Firefox 9
x86
Linux
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 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

6 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

6 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

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

Comment 3

6 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?

Comment 4

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

Updated

6 years ago
Priority: P1 → P2
(Assignee)

Comment 5

6 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

6 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

6 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

6 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.

Updated

6 years ago
Attachment #556931 - Flags: review?(dao)
(Assignee)

Comment 12

6 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

6 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: 6 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
You need to log in before you can comment on or make changes to this bug.