Closed
Bug 665907
Opened 14 years ago
Closed 13 years ago
[highlighter] Bounding boxes update are slow while the page is scrolling.
Categories
(DevTools :: General, defect, P2)
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)
6.37 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
Summary: [highlighter] Bounding boxes are slow while the page is scrolling. → [highlighter] Bounding boxes update are slow while the page is scrolling.
Comment 1•14 years ago
|
||
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•14 years ago
|
Whiteboard: [minotaur]
Updated•14 years ago
|
Keywords: polish
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][best: 1h; likely: 4h; worst: 2d]
Updated•14 years ago
|
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Assignee | ||
Updated•13 years ago
|
Assignee: rcampbell → mihai.sucan
Assignee | ||
Comment 3•13 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•13 years ago
|
||
Could probably just disable transitions once the node is locked, and enable them again when unlocked.
Updated•13 years ago
|
Priority: P1 → P2
Assignee | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Dão: btw, thanks a lot for your really quick response to the patch!
Comment 9•13 years ago
|
||
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•13 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?
Comment 11•13 years ago
|
||
I think at the point where the JS gets messy, you should use the child selector.
Updated•13 years ago
|
Attachment #556931 -
Flags: review?(dao)
Assignee | ||
Comment 12•13 years ago
|
||
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 13•13 years ago
|
||
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•13 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 15•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [minotaur][best: 1h; likely: 4h; worst: 2d][land-in-fx-team] → [minotaur][best: 1h; likely: 4h; worst: 2d][fixed-in-fx-team]
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 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•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•