Closed Bug 66619 Opened 19 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: jruderman, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: access, helpwanted)

Attachments

(3 files, 9 obsolete files)

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.
Status: NEW → ASSIGNED
OS: Windows 98 → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → Future
this would be nifty...
Keywords: helpwanted
Keywords: access, fcc508
Reassigning alecf keyboard bugs.

Chris, who should get this?
Assignee: alecf → saari
Status: ASSIGNED → NEW
Severity: enhancement → normal
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?
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.
Severity: normal → minor
Blocks: focusnav
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?
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...
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.
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.
Attached patch purpose to fix (obsolete) — Splinter Review
Just make all frames in same content visible when tab into the content.
Attachment #79609 - Attachment description: popuse to fix → purpose to fix
Attached patch Fix some problem (obsolete) — Splinter Review
Use original frame's height to increase frameBounds' height
Attachment #79609 - Attachment is obsolete: true
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.
Attachment #79786 - Attachment is obsolete: true
Oops, this never got reviewed!
Chris, could you r= this?
Comment on attachment 80935 [details] [diff] [review]
use nextframe's height to increase frameBounds' height

r=saari
Attachment #80935 - Flags: review+
aaron, giving this to you for landing. Thanks
Assignee: saari → aaronl
=>myself
Sorry... I almost forget this bug :-P
I will seeking sr= ASAP
Assignee: aaronl → pete.zha
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.
cc'ing kin and dbaron, who can sanity check me here.
Comment on attachment 80935 [details] [diff] [review]
use nextframe's height to increase frameBounds' height

needs work, if next-in-flow idea is better.
Attachment #80935 - Flags: needs-work+
Attached patch patch (use next-in-flow) (obsolete) — Splinter Review
OK, change to use (next-in-flow)
waterson, please sr= this one. Thanks!
Attachment #80935 - Attachment is obsolete: true
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);
>+        }
>+      }
Attachment #89393 - Flags: needs-work+
Attached patch patch (use UnionRect) (obsolete) — Splinter Review
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.
Attachment #89393 - Attachment is obsolete: true
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.
Attachment #89500 - Flags: needs-work+
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.)
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?
Assignee: pete.zha → mwulffeld
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.
Attachment #89500 - Attachment is obsolete: true
*** Bug 185163 has been marked as a duplicate of this bug. ***
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.
Attachment #111235 - Attachment is obsolete: true
Patch against -current.
Attachment #111511 - Attachment is obsolete: true
Attachment #123782 - Flags: review?(dbaron)
What is it that you're trying to fix with the ...SCROLLFLAG_SIBLINGS stuff?  I
don't see why that should be necessary.
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.
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?
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?
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.
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 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.
Attachment #123782 - Flags: review?(dbaron) → review-
Attached patch Patch v0.3 (obsolete) — Splinter Review
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().
Attachment #123782 - Attachment is obsolete: true
Attachment #129729 - Flags: review?(dbaron)
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...
You're very welcome to.

Reassigning.
Assignee: martin → dbaron
David?
> Perhaps I should just take this over...

I think you should reassign it back to Martin, if he's still interested. Or?
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.
...which is basically fixing bug 233164 correctly.  See also bug 50511 comment 64.
Blocks: 332246
See also bug 211854, same bug for textareas.
Attached patch patchSplinter Review
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.
Attachment #129729 - Attachment is obsolete: true
Attachment #271290 - Flags: superreview?(roc)
Attachment #271290 - Flags: review?(sharparrow1)
Attachment #129729 - Flags: review?(dbaron)
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...
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.
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 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 :)
Attachment #271290 - Flags: review?(sharparrow1) → review+
Attachment #271290 - Flags: superreview?(roc) → superreview+
I checked this in to the trunk earlier today; still need to file followup bugs.
Depends on: 388019
I backed this out until I get review on bug 388019.

I also have a mochitest for it on that bug.
Checked in to trunk again.
Flags: in-testsuite+
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9beta1
No.  I'll reland it once the tree opens.
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.