66619 - tabbing to a multi-line link should try to make entire link visible

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Jesse Ruderman]

Currently, if I tab to a multi-line link on http://www.mozilla.org/, Mozilla 
only makes the first line visible.  The page should scroll far enough to make 
the entire link visible, favoring showing the top of the link if the link is 
very tall.

Comment 1

[sairuh (rarely reading bugmail)]

this would be nifty...

Comment 2

[Aaron Leventhal]

Reassigning alecf keyboard bugs.

Chris, who should get this?

Comment 3

[saari (gone)]

This logic isn't too hard, pretty much anyone could own this. It is just a
matter of not only using GetPrimaryFrameFor

aaronl, how important is this?

Comment 4

[Aaron Leventhal]

I suppose it's not all that important in the scheme of things.
It could be considered a real accessibility problem at some point - once we
clear off the more important ones.

Comment 5

[Jim Song]

Hi, Jesse,
I test the www.mozilla.org,
found that the page would be scolled and 
the multilink line be shown totally. Could
you give us a test case, or just point
out the simple reproduce step?

Comment 6

[Pete Zha]

Works for me. If I tab into a multi-line link like "Develope Doc". browser will 
make the entire link visible. Maybe has been fixed...

Comment 7

[Aaron Leventhal]

Still a problem for me.

Here ar e my steps to reproduce:

1. Load http://lxr.mozilla.org/seamonkey
2. Resize your screen so that only "File" from "File Name Search" appears on the
bottom.
3. Click in the first text field and tab twice. Only the "File" part of "File
Name Search" will be visible on screen.

Comment 8

[Pete Zha]

This is because GetPrimaryFrameFor will only return the first frame(maybe the 
topest one?) of the content... So, only the first frame will be scroll to view.

Comment 9

[Pete Zha]

Attachment: purpose to fix

Just make all frames in same content visible when tab into the content.

Comment 10

[Pete Zha]

Attachment: Fix some problem

Use original frame's height to increase frameBounds' height

Comment 11

[Aaron Leventhal]

We should not assume that all of the frames for the current content node have
the same height.

Instead of adding frameHeight each time through the loop, we should add
lastFrame.height, or something like that.

Comment 12

[Pete Zha]

Attachment: use nextframe's height to increase frameBounds' height

Comment 13

[Aaron Leventhal]

Oops, this never got reviewed!
Chris, could you r= this?

Comment 14

[saari (gone)]

Comment on attachment 80935 [details] [diff] [review]
use nextframe's height to increase frameBounds' height

r=saari

Comment 15

[saari (gone)]

aaron, giving this to you for landing. Thanks

Comment 16

[Pete Zha]

=>myself
Sorry... I almost forget this bug :-P
I will seeking sr= ASAP

Comment 17

[Chris Waterson]

A better way to do this (IMO) would be to determine if the frame is splittable
(i.e., can have continuations) by calling nsIFrame::IsSplittable, and then if
so, calling nsIFrame::GetNextInFlow (and adding the NIF's area) until there are
no more next-in-flows.

Comment 18

[Chris Waterson]

cc'ing kin and dbaron, who can sanity check me here.

Comment 19

[Chris Waterson]

Comment on attachment 80935 [details] [diff] [review]
use nextframe's height to increase frameBounds' height

needs work, if next-in-flow idea is better.

Comment 20

[Pete Zha]

Attachment: patch (use next-in-flow)

OK, change to use (next-in-flow)
waterson, please sr= this one. Thanks!

Comment 21

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

Comment on attachment 89393 [details] [diff] [review]
patch (use next-in-flow)

>+      nsSplittableType isSplittable;
>+      aFrame->IsSplittable(isSplittable);
>+      if (NS_FRAME_SPLITTABLE == isSplittable) {
>+        nsIFrame* nextFrame;
>+        nsresult rv = aFrame->GetNextInFlow(&nextFrame);

You probably don't need to check IsSplittable -- that's for a caller that wants
to create a next-in-flow.  You can just call GetNextInFlow directly without
checking IsSplittable.

>+        while (NS_SUCCEEDED(rv) && nextFrame) {
>+          nsRect nextFrameBounds;
>+          nextFrame->GetRect(nextFrameBounds);
>+          frameBounds.height += nextFrameBounds.height;

You should be using UnionRect (i.e.,
|frameBounds.UnionRect(frameBounds,nextFrameBounds)| rather than adding to the
height, since there could be a line-height or an image in the line that makes
the frames separated.  However, that would require that you do tho
offset-from-view correction first.  Doing that will also fix the fact that you
do the wrong thing if the frame's parent is split.  You also want to be doing
this in a way that the FindLineContaining fixup happens for each of the split
parts.	In other words, you want this loop over next-in-flow frames to cover a
much larger section of the code (which could perhaps be refactored into a
separate static function?).

>+          rv = nextFrame->GetNextInFlow(&nextFrame);
>+        }
>+      }

Comment 22

[Pete Zha]

Attachment: patch (use UnionRect)

David,
So, what's your suggestion? I was doing a wrong thing? Or just need to use
UnionRect instead add height directly?
I have a new patch here. In this patch, I set frameBounds' x/y with offset.x/y
- oldBounds.x/y. This is because I want to move frameBounds to correct position
after unite all frames in flow.

Comment 23

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

Comment on attachment 89500 [details] [diff] [review]
patch (use UnionRect)

I don't see how this is going to do the right thing when the frame's parent is
split.	You really need to move the merging logic outside the next loop out.

Comment 24

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

Every frame is in coordinates relative to its parent.  Suppose that you
have something like

  <p><span>This is some text with a <a>long anchor that ends up being
  broken across multiple lines</a>.</span></p>

If there's a line break in the middle of the anchor, then the parent of
the continuing frame for the anchor will be the continuing frame for the
span.  So it will have an origin of (0,0).  In other words, the
coordinates for the continuing frame for the anchor will be in a
different coordinate space than the coordinates for the primary frame
for the anchor.  This means that you really need to make the coordinates
absolute relative to the scroll frame (or document, or something like
that -- I'm not exactly sure) *before* you try to union them together.
It's also good to get the code that causes the whole line to be included
for every frame, not just the primary frame.

This means the new loop should probably start between the lines:

      scrollingView->GetScrolledView(scrolledView);
      aFrame->GetOffsetFromView(mPresContext, offset, &closestView);

and end between the lines:

        closestView->GetParent(closestView);
      }

      // Determine the visible rect in the scrolled view's coordinate space.

(I think, at least.)

Comment 25

[Martin Wulffeld]

Taking. Have a patch real soon.

Please take a look at 185163. Am I right in thinking the implementation fix for
that bug is different?

Comment 26

[Martin Wulffeld]

Attachment: Patch - Handles 0,0 origin cases.

I took Pete's last patch and modified it so the things dbaron requested are in.
It should now handle those cases where the parent origin is 0,0. However, I'm
definately not comfortable with the code in this function and I'm not sure the
loop grabs as much code as it should or it breaks something else. Review with
extreme care.

The changes to the function headers in the end are merely stylistic so they
match the rest of the code.

Comment 27

[Martin Wulffeld]

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

Comment 28

[Martin Wulffeld]

Attachment: Patch (uses GetNextInFlow() or GetNextSibling())

Added extra argument to ScrollFrameIntoView() so the caller may specify whether
to iterate over next-in-flow or siblings. Default flag is set to iterate over
next-in-flows. Not sure if this is desirable. Also not sure which callers would
want to iterate over next-in-flows and which would not so I haven't touched any
of those.

The NS_PRESSHELL_SCROLLFLAG_SIBLING flag is for the call from
nsEventStateManager::ShiftFocusInternal() to work since next-in-flows would do
no good here as far as I can see.

Will add testcase later today.

Comment 29

[Martin Wulffeld]

Attachment: Testcase (Long links that wrap and image links which should all scroll into view)

Comment 30

[Martin Wulffeld]

Attachment: Patch (uses GetNextInFlow() or GetNextSibling()) - v0.2

Patch against -current.

Comment 31

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

What is it that you're trying to fix with the ...SCROLLFLAG_SIBLINGS stuff?  I
don't see why that should be necessary.

Comment 32

[Martin Wulffeld]

That is for the cases such as:

<p>
 <a href="link">
  <img width="50" src="http://www.ballooninaboxusa.com/soccer.gif"><img
style="position: relative; top: 100px;" width="50"
src="http://www.ballooninaboxusa.com/soccer.gif">
 </a>
</p>

which I suppose represents a subtree in Mozilla (the images). I have to iterate
over this subtree with GetNextSibling() since it's not a flow like a regular
wrapped text link would be. Otherwise only the first image would be scrolled
into view.

Caveat emptor. I don't know the code very well so I might have completely missed
something.

Comment 33

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

I'm assuming (in the example in comment 32) that you meant '<a name="link">'
instead of '<a href="link">'.  It doesn't make any sense to me that you'd need
|GetNextSibling| to handle that.  Do you have a testcase?

Comment 34

[Martin Wulffeld]

Attachment: Testcase showing need of GetNextSibling() call

This testcase should show what I mean. If you remove the SCROLLFLAG_SIBLINGS
flag from the following call in
content/events/src/nsEventStateManager.cpp#3271:

  presShell->ScrollFrameIntoView(child,
				 NS_PRESSHELL_SCROLL_ANYWHERE,
				 NS_PRESSHELL_SCROLL_ANYWHERE,
				 NS_PRESSHELL_SCROLLFLAG_SIBLINGS);

you should see that the butterfly image is not scrolled into view when you tab
through this testcase?

Comment 35

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

Shouldn't you always be walking through all the siblings when you're doing the
subtree of the frame, but never for the frame itself?  I don't see how this
should be part of the API.

Comment 36

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

Oh, I see now that you're extracting the first child at the caller?  Why should
the caller have to do that?  It makes the API unnecessarily complicated and
presumably doesn't fix similar problems for other callers.

Comment 37

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

Comment on attachment 123782 [details] [diff] [review]
Patch (uses GetNextInFlow() or GetNextSibling()) - v0.2

I don't think there should be any reason to complicate the API with the
scrollflag stuff.  The code you added in nsEventStateManager.cpp should just be
able to use the parent rather than using SCROLLFLAG_SIBLINGS after explicitly
getting the first child.

Comment 38

[Martin Wulffeld]

Attachment: Patch v0.3

Finally got around to doing something with this again. Sorry for the wait.

This patch should address your comments. I initially thought I had to preserve
the original functionality of ScrollFrameIntoView() hence the scrollflag stuff.
That's all gone now and the check to use Sibling() or Flow() has been put
inside ScrollFrameIntoView().

Comment 39

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

Comment on attachment 129729 [details] [diff] [review]
Patch v0.3

>+  PRBool hasOutsideChildren = PR_FALSE;
>+  nsFrameState state;
>+  aFrame->GetFrameState(&state);
>+  if (state & NS_FRAME_OUTSIDE_CHILDREN) {
>+    aFrame->FirstChild(mPresContext, nsnull, &aFrame);
>+    hasOutsideChildren = PR_TRUE;
>+  }

This doesn't make sense.  Just because a frame has children extending outside
of it doesn't mean that they extend outside of it in all directions.  You need
to get the frame's bounds too.

And we probably only want to do the line business once, for the outermost.

Perhaps I should just take this over...

Comment 40

[Martin Wulffeld]

You're very welcome to.

Reassigning.

Comment 41

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

See also bug 195905.

Comment 42

[Aaron Leventhal]

David?
> Perhaps I should just take this over...

I think you should reassign it back to Martin, if he's still interested. Or?

Comment 43

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

Bug 195905 comment 16 suggests to me that we need to make click-to-focus (rather
than tab-to-focus) not scroll before we try to fix this bug.

Comment 44

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

...which is basically fixing bug 233164 correctly.  See also bug 50511 comment 64.

Comment 45

[Jesse Ruderman]

See also bug 211854, same bug for textareas.

Comment 46

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

Attachment: patch

Sorry it took me 4 years to get to this...

Note that we're still using ...SCROLL_IF_NOT_VISIBLE, which means links won't be scrolled into view if they're already partially visible.  I don't like that, but that's a separate patch entirely... and the stuff mentioned in the previous few comments.

Note that I haven't actually tested the case of a link split between different views, but I'll try to do that shortly by hacking nsContainerFrame::FrameNeedsView.

Comment 47

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

Yep, if I hack FrameNeedsView to make frames for elements with opacity < 1, and give either the anchors in the testcase or spans containing them opacity, then things still work.  (And the same testcases don't work if I additionally comment out my view manipulation at the end of UnionRectForNearestScrolledView.)

Note that another followup issue that should be filed is doing this for NS_FRAME_IS_SPECIAL cases...

Comment 48

[Eli Friedman]

I'm very suspicious of the code in the "if (frameBounds.height == 0 || aVPercent != NS_PRESSHELL_SCROLL_ANYWHERE)" bit; it looks like it doesn't deal with views correctly.

Actually, wouldn't it make a lot more sense to avoid using views altogether?  It would be just as straightforward to write this in terms of frames, and we'll have to do it at some point anyway to get rid of views.

Comment 49

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

Why don't you think it deals with views correctly?  It looks fine to me.  Either way, this patch doesn't touch that code -- it just moves it into a separate function so that we can loop over continuations.

Comment 50

[Eli Friedman]

Comment on attachment 271290 [details] [diff] [review]
patch

Mostly because of the "if (blockView == closestView) {" bit, which in theory isn't guaranteed to be true.  But it isn't really that important, and I won't try to make you rewrite it :)

Comment 51

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

I checked this in to the trunk earlier today; still need to file followup bugs.

Comment 52

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

I backed this out until I get review on bug 388019.

I also have a mochitest for it on that bug.

Comment 53

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

Checked in to trunk again.

Comment 54

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

I filed bug 388284 on scrolling partially visible links fully into view (which I think is actually a regression sometime in the past few years -- well after this bug was originally filed).

Marking fixed.

Comment 55

[Neil Deakin]

dbaron: I assume the one line change at the end of http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/base&command=DIFF_FRAMESET&file=nsPresShell.cpp&rev2=3.1034&rev1=3.1033
wasn't intentional?

Comment 56

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

No.  I'll reland it once the tree opens.

Comment 57

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381