289250 - FrameTraversal broken

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Doug Turner (:dougt)]

I think resent changes to layout broken the following code.  I am not sure if
this code is buggy and needs to be rewritten, or a regression occured.  Please
advise:

GIVEN A |nsPresContext|:


  nsCOMPtr<nsIDocument> doc = content->GetDocument();
  if (!doc) return nsnull;
  
  // the only case where there could be more shells in printpreview
  nsIPresShell *shell = doc->GetShellAt(0);
  if (!shell) return nsnull;
  
  nsPresContext *presContext = shell->GetPresContext();


The following code yields an enumerator with very few (~3) frames in it.  Before
recent changes to layout, this enumerator would be full of frame (frames for the
entire presContext).

nsresult createFrameTraversal(PRUint32 type, /* IS ALWAYS FOCUS */
                              nsPresContext* presContext, 
                              nsIBidirectionalEnumerator** outTraversal)
{
  nsresult result;
  if (!presContext)
    return NS_ERROR_FAILURE;    
  
  nsIPresShell* presShell = presContext->PresShell();
  if (!presShell)
    return NS_ERROR_FAILURE;
  
  nsIFrame* frame = nsnull;
#ifdef OLDER_LAYOUT
  presShell->GetRootFrame(&frame);
#else  
  frame = presShell->GetRootFrame();
#endif
  
  if (!frame)
    return NS_ERROR_FAILURE;
  
  nsCOMPtr<nsIBidirectionalEnumerator> frameTraversal;
  
  static NS_DEFINE_CID(kFrameTraversalCID, NS_FRAMETRAVERSAL_CID);
  nsCOMPtr<nsIFrameTraversal> trav(do_CreateInstance(kFrameTraversalCID,&result));
  if (NS_FAILED(result))
    return result;
  
  result = trav->NewFrameTraversal(getter_AddRefs(frameTraversal),
                                   type, 
                                   presContext, 
                                   frame);
  if (NS_FAILED(result))
    return result;
  
  NS_IF_ADDREF(*outTraversal = frameTraversal);
  return NS_OK;
}

Thanks for looking into this.

Comment 1

[Doug Turner (:dougt)]

forgive the OLDER_LAYOUT define.  it isn't set in our case.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm... The frame traversal code for FOCUS doesn't really look at scrollframes
(unlike EXTENSIVE and LEAF)....  roc, any idea what could be up here?  Somehow
the frame iterator is clearly getting confused by the changes to the root frame
setup...

Comment 3

[Doug Turner (:dougt)]

updating my tree back to 2005-03-20 08:00:00 fixes the problem.

Comment 4

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

Attachment: potential fix

The problem is that we reordered the scrollframe children so that scrollbars
are first, followed by the canvas frame. The frames look like this:
== viewport
  == scrollframe
    == scrollbar
    == scrollbar
    == canvas
      == block and rest of document

So what happens in focus frame traversal is, we return the viewport frame, then
we return the scrollframe, then we return the scrollbars, then the
sibling/parent traversal loop sees IsRootFrame is true and thinks it's done.
The problem is that we should only be checking IsRootFrame after we've moved up
to the parent. A couple of the other methods have similar problems, although
they already have the necessary check after GetParent() too. This patch fixes
them all.

Doug, please test this patch and let me know if it works for you...

Comment 5

[Doug Turner (:dougt)]

roc, that didn't work. :-(

Comment 6

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

Can you be more specific?

After you get the root frame, you could add a statement

    frame->List(frame->GetPresContext(), stdout, 0);

to dump the frame tree, and then add some other printfs to print the frame
pointers you get from the iterator (e.g., iter->CurrentItem(&f); printf("got
frame %x\n", f); ). That output would help to debug this...

Comment 7

[Doug Turner (:dougt)]

requesting blocking status.

working on getting roc debugging data.

Comment 8

[Doug Turner (:dougt)]

Attachment: Log

roc, this is the log per your request.

The page used to test this was, for reference, was
http://www.google.com/firefox?client=firefox-a&rls=org.mozilla:en-US:official

Comment 9

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

Attachment: potential fix #2

Comment 10

[Doug Turner (:dougt)]

roc, that patch works for me.  i will continue testing to see if anything is
still broken.  Great work.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So what's that patch actually doing, and why?

Comment 12

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

The frame traversal used to stop whenever it reached a frame matching
IsRootFrame (e.g., the canvas frame, which is a child of the viewport's
scrollframe). Now it only stops when we reach that frame when we back up from a
child frame to its parent.

What this means is that if you start a frame traversal at the root (viewport)
frame, you will see all frames. If you start a frame traversal from inside the
canvas frame, it will not enumerate the canvas frame or anything outside it (the
iewport frame, the viewport scrollframe, or the viewport scrollframe's
scrollbars and scrollcorner). Basically it's the same behaviour as before we
regressed except that if you initiate a frame traversal at the root, you now see
 the viewport scrollframe's scrollbars and scrollcorner, but you didn't before.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181466 [details] [diff] [review]
potential fix #2

>Index: layout/base/nsFrameTraversal.cpp

nsLeafIterator::Prev doesn't seem to have an IsRootFrame() check.  Should it?

> nsFocusIterator::Next()

>+        if (IsRootFrame(result)) {
>+          result = 0;
>+          break;

Do you need the break?	Won't that happen automatically?

> nsFocusIterator::Prev()

That method needs to do an IsRootFrame() check too, no?

Same for nsVisualIterator::Prev.

r=bzbarsky as a regression fix, but this only "unregresses" us to a broken
state, as far as I can tell -- going back and forward don't do quite the same
thing....

Comment 14

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

(In reply to comment #13)
> > nsFocusIterator::Next()
> 
> >+        if (IsRootFrame(result)) {
> >+          result = 0;
> >+          break;
> 
> Do you need the break?	Won't that happen automatically?

It will. We could remove the break.

> > nsFocusIterator::Prev()
> 
> That method needs to do an IsRootFrame() check too, no?
> 
> Same for nsVisualIterator::Prev.
> 
> r=bzbarsky as a regression fix, but this only "unregresses" us to a broken
> state, as far as I can tell -- going back and forward don't do quite the same
> thing....

Using IsRootFrame instead of != nsnull used to have the effect of stopping
enumeration without returnng the viewport scrollbars. I have no idea why that
might have been important.

Prev didn't need IsRootFrame because the scrollbars were after all the body
frames, so starting at a body frame wouldn't see the scrollbars anyway. Now,
because the viewport scrollbars come before the body frames, Prev will be seeing
them, and so is Next (and without the patch, if you start iterating at the
viewport frame, that's all you see). Again, I have no idea if this is a problem
for any frame traversal user.

Replacing all the IsRootFrame checks with null checks would make Next and Prev
symmetric and the only change would be that you always see the viewport
scrollbars. It seems to me that that's the simplest and least surprising thing
to do. Should we do that?

Comment 15

[Doug Turner (:dougt)]

patch 2 worked for me.  i can test out any other patch you produce.

Comment 16

[Boris Zbarsky [:bzbarsky]]

That makes some sense to me, but I also don't know what frame traversal users
that could break...  Tabbing has me particularly worried.

Comment 17

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

I think we can push this out to b3.

Comment 18

[Doug Turner (:dougt)]

disappointing, but understandable.  lets try for 1.8b3

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 181466 [details] [diff] [review]
potential fix #2

moving request out to b3 for consideration after b2 ships.

Comment 20

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181466 [details] [diff] [review]
potential fix #2

a=shaver

Comment 21

[Doug Turner (:dougt)]

Checking in nsFrameTraversal.cpp;
/cvsroot/mozilla/layout/base/nsFrameTraversal.cpp,v  <--  nsFrameTraversal.cpp
new revision: 3.45; previous revision: 3.44
done

Fix checked in.