The default bug view has changed. See this FAQ.

highlighter and infobar should not be visible if node is not highlightable

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dcamp, Assigned: paul)

Tracking

9 Branch
Firefox 10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Comment 1

6 years ago
Also, it feels like the box animates while it shrinks down to that pixel size, aren't the animations disabled while locked?
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 2

6 years ago
I found another situation where the highlighter and/or the infobar are still visible when they should not (selecting a non-highlightable node after a highlightable node).
Summary: Tiny highlighter rect still visible when element is scrolled offscreen. → highlighter and infobar should not be visible if node is not highlightable
(Assignee)

Comment 3

6 years ago
I think the infobar should be move to the top/left corner, bot be hidden.
(Assignee)

Comment 4

6 years ago
Created attachment 566151 [details] [diff] [review]
patch v1

- hide the highlighter borders if node not highlitable
- move the infobar to the top-left corner if node not highlitable
Attachment #566151 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Comment on attachment 566151 [details] [diff] [review]
patch v1

     // node is not set or node is not highlightable, bail
-    if (!this.node || !this.isNodeHighlightable(this.node)) {
-      return;
-    }
+    if (this.node && this.isNodeHighlightable(this.node)) {

I think I'd prefer to bail out early rather than nest the remainder of this method in an if { } block.

r+ with that reverted.
Attachment #566151 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 6

6 years ago
Well, actually, we don't want to bail out here. We want to execute:

>    this.highlightRectangle(rect);
>    this.moveInfobar();

(after the if block).

I think here I should only remove the comment.
(Assignee)

Updated

6 years ago
Blocks: 663830
(In reply to Paul Rouget [:paul] from comment #6)
> Well, actually, we don't want to bail out here. We want to execute:
> 
> >    this.highlightRectangle(rect);
> >    this.moveInfobar();
> 
> (after the if block).
> 
> I think here I should only remove the comment.

OK, fair enough.

One thing I would like to see fixed though is the while(true) block. Can we replace that with something like:

while (!(frameWin.parent === frameWin || !frameWin.frameElement)) {

?

or do { ... } while() ?
(Assignee)

Comment 8

6 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> (In reply to Paul Rouget [:paul] from comment #6)
> > Well, actually, we don't want to bail out here. We want to execute:
> > 
> > >    this.highlightRectangle(rect);
> > >    this.moveInfobar();
> > 
> > (after the if block).
> > 
> > I think here I should only remove the comment.
> 
> OK, fair enough.
> 
> One thing I would like to see fixed though is the while(true) block. Can we
> replace that with something like:
> 
> while (!(frameWin.parent === frameWin || !frameWin.frameElement)) {
> 
> ?
> 
> or do { ... } while() ?

I don't see how I could do that.
ok, after another look, it looks like it'll be a pain to move that around, so don't worry about it.
(Assignee)

Comment 10

6 years ago
Created attachment 568414 [details] [diff] [review]
patch v1.1

removed the comfusing comment
(Assignee)

Updated

6 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/f9644e6e81d5
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f9644e6e81d5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.