66617 - [regression] if tabbing scrolls page, focus outline doesn't leave first link

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[Jesse Ruderman]

1. Open http://www.mozilla.org/.
2. Right-click on one of the links at the left.
3. Press tab a bunch of times (but don't hold it).

Result: each time pressing tab causes the page to scroll, the focus outline 
isn't removed from the link that was just un-focused.

This bug occurs on 012504 but not on 011904.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Kevin McCluskey (gone)]

Reassigning to Rod.

Comment 3

[rods (gone)]

This is a painting issue, and is either caused by some focus code changing or 
something in core layout.

Comment 4

[Jesse Ruderman]

*** Bug 69322 has been marked as a duplicate of this bug. ***

Comment 5

[Marc Attinasi]

Rod, do you have any information about why this painting issue must be a core
layout or selection code problem? Seems to me that core layout has little to do
with painting when the view scrolls... I don't have time to look at this, and it
is not my area of experteise anyway. Please provide any information you gathered
before reassigning it, that would be very helpful - thanks.

Comment 6

[Marc Attinasi]

No response from Rod... OK, Kevin, can you provide any clues about how scrolling 
might be messing up what gets invalidated? My guess is that the invalidation is 
happening before the scroll, so that the region invalidated is not valid after 
the scroll. This sounds like a View issue (maybe?) I don't even know where to 
start as this is not my area. I think this is reasonably important, however.

Comment 7

[Blake Ross]

If I recall correctly, this started when saari checked in his one liner to fix 
tabbing from multiline links.

Comment 8

[saari (gone)]

Uh, I kinda doubt that. That particular checkin didn't change anything related
to rendering, just what nsEventStateManager::GetNextTabbable content choose as
the next item. Now some other checkin of mine is entirely possible

Comment 9

[Marc Attinasi]

This could be an issue with views for all I know, but I'll have to look more to 
know. Not critical right now, so off to Mozilla1.0

Comment 10

[sairuh (rarely reading bugmail)]

i see this on linux.

Comment 11

[Aaron Leventhal]

Adding fcc508 keyword - needed for legal compliance in US government purchasing
contracts.

Comment 12

[Pietro Pino]

Attachment: A workaround patch which prevents automatic scrolling on the newly focused element

Comment 13

[Ken]

I realize that patch is a workaround, but we should strive to keep the 
automatic scrolling...a very nice feature. There must be a fix to remove the 
outline of the last focused item before setting on the current focus item.

Come to think of it, this problem seems unrelated to the automatic scrolling. 
It is a much broader focus problem. Try this:
   Start mfcembed
   Select some text
   Press tab a few times. 
   = Note how the text never loses focus, even as you tab through the links.

Please do not apply the patch to prevent automatic scrolling and try to 
identify the source of the focus problem.

Comment 14

[Ken]

Found <A HREF="http://bugzilla.mozilla.org/show_bug.cgi?id=56472">56472</A> to 
describe that condition.

There is a number of related and/or dependent focus bugs too :(, like
<A HREF="http://bugzilla.mozilla.org/show_bug.cgi?id=36848">36848</A>, 
<A HREF="http://bugzilla.mozilla.org/show_bug.cgi?id=83496">83496</A>, 
<A HREF="http://bugzilla.mozilla.org/show_bug.cgi?id=28583">28583</A>

Comment 15

[Ken]

Found http://bugzilla.mozilla.org/show_bug.cgi?id=56472 to 
describe that condition.

There is a number of related and/or dependent focus bugs too :(, like
http://bugzilla.mozilla.org/show_bug.cgi?id=36848 
http://bugzilla.mozilla.org/show_bug.cgi?id=83496 
http://bugzilla.mozilla.org/show_bug.cgi?id=28583

Comment 16

[Judson Valeski]

topembed . we need to get our focus story focused :-)

Comment 17

[Brian Ryner (not reading)]

Attachment: patch

Comment 18

[Judson Valeski]

you have a raw pointer to a nsIFrame w/ out releasing, intentional?

Comment 19

[Brian Ryner (not reading)]

yes, frames aren't refcounted.

Comment 20

[Judson Valeski]

r=valeski. someone w/ more localized expertise should r/sr this too though.

Comment 21

[saari (gone)]

r=saari

Comment 22

[David Hyatt]

sr=hyatt

Comment 23

[Brian Ryner (not reading)]

Fixed on trunk, keeping bug open for branch checkin.

Comment 24

[Brian Ryner (not reading)]

Ok, this doesn't seem to work in all cases.  Investigating.

Comment 25

[Brian Ryner (not reading)]

Attachment: follow-up patch

Comment 26

[Brian Ryner (not reading)]

Ok, I was using the wrong coordinate space when invalidating the frame.  This
seems to fix the problem.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron, although it would have been nice if the first patch were less of a
hack (i.e., maybe nsScrollingView needs to translate queued invalidate regions
when it scrolls by blitting or something like that)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And since a key part of the fix is in nsHTMLAnchorElement, I somehow suspect
the current fix won't work for imagemaps and form controls.)

Comment 29

[Dan M]

May I suggest this modification to the last patch? GetPrimaryFrameFor's
(reasonable) assertions are driving me crazy.

Index: nsHTMLAnchorElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp,v
retrieving revision 1.80
diff -u -r1.80 nsHTMLAnchorElement.cpp
--- nsHTMLAnchorElement.cpp	2001/08/10 23:50:05	1.80
+++ nsHTMLAnchorElement.cpp	2001/08/13 19:30:54
@@ -305,13 +305,15 @@
         if (frame) {
           presShell->ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_ANYWHERE,
                                          NS_PRESSHELL_SCROLL_ANYWHERE);
-          nsIFrame* oldFocusFrame = nsnull;
-          presShell->GetPrimaryFrameFor(oldFocus, &oldFocusFrame);
-          if (oldFocusFrame) {
-            // Invalidate the area that the old frame covers
-            nsRect rect;
-            oldFocusFrame->GetRect(rect);
-            oldFocusFrame->Invalidate(aPresContext, rect, PR_FALSE);
+          if (oldFocus) {
+            nsIFrame* oldFocusFrame = nsnull;
+            presShell->GetPrimaryFrameFor(oldFocus, &oldFocusFrame);
+            if (oldFocusFrame) {
+              // Invalidate the area that the old frame covers
+              nsRect rect;
+              oldFocusFrame->GetRect(rect);
+              oldFocusFrame->Invalidate(aPresContext, rect, PR_FALSE);
+            }
           }
         }
       }

Comment 30

[Brian Ryner (not reading)]

I think I know what the real fix is for this in nsScroll[ing|Port]View... we
need to offset our dirty region when we scroll (since the dirty region is in
absolute screen coordinates).  Waiting to hear back from roc about exactly when
we should do this.

Comment 31

[Brian Ryner (not reading)]

Attachment: new patch

Comment 32

[Brian Ryner (not reading)]

Oops, that could crash if the dirty region hasn't been initialized for some
reason.  Safer patch coming up.

Comment 33

[Brian Ryner (not reading)]

Attachment: patch v2.1

Comment 34

[Brian Ryner (not reading)]

Ok, this patch reverts my previous patch and replaces it with what I think is
the right fix.  Here is my understanding of the problem:

Each view keeps a dirty region which will be repainted the next time it receives
an invalidate event (invalidates are usually done asynchronously).  The dirty
region is stored as absolute screen coordinates.  So, when we scroll the view,
we need to offset this dirty region by the amount we scrolled so that the
correct frames are repainted.

Looking for reviews...

Comment 35

[Ken]

looks good to me. when can this go to the branch?

Comment 36

[saari (gone)]

The patch looks reasonable to me, but I do want roc to look at it

Comment 37

[David Hyatt]

r/sr=hyatt

Comment 38

[saari (gone)]

r=saari. Robert O'Calahan can comment later if he wishes, but time is short so
in she goes.

Comment 39

[Brian Ryner (not reading)]

ok, let's try this again...

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I was out of town all week at a workshop.

The fix looks good.

Comment 41

[Brian Ryner (not reading)]

Checked in on the branch and trunk.

Comment 42

[Chris Petersen]

Marking verified in the Sept 05 th build (2001-09-05-05)