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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Keyboard: Navigation
P3
minor
RESOLVED FIXED
18 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {access, helpwanted, sec508})

Trunk
mozilla1.9alpha8
access, helpwanted, sec508
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

18 years ago
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.

Updated

18 years ago
Status: NEW → ASSIGNED
OS: Windows 98 → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → Future
this would be nifty...
Keywords: helpwanted

Updated

17 years ago
Keywords: access, fcc508

Comment 2

17 years ago
Reassigning alecf keyboard bugs.

Chris, who should get this?
Assignee: alecf → saari
Status: ASSIGNED → NEW

Updated

17 years ago
Severity: enhancement → normal

Comment 3

17 years ago
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

17 years ago
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

Updated

17 years ago
Blocks: 83552

Comment 5

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Created attachment 79609 [details] [diff] [review]
purpose to fix

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

Updated

17 years ago
Attachment #79609 - Attachment description: popuse to fix → purpose to fix

Comment 10

17 years ago
Created attachment 79786 [details] [diff] [review]
Fix some problem

Use original frame's height to increase frameBounds' height
Attachment #79609 - Attachment is obsolete: true

Comment 11

17 years ago
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

17 years ago
Created attachment 80935 [details] [diff] [review]
use nextframe's height to increase frameBounds' height
Attachment #79786 - Attachment is obsolete: true

Comment 13

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

Comment 14

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

r=saari
Attachment #80935 - Flags: review+

Comment 15

16 years ago
aaron, giving this to you for landing. Thanks
Assignee: saari → aaronl

Comment 16

16 years ago
=>myself
Sorry... I almost forget this bug :-P
I will seeking sr= ASAP
Assignee: aaronl → pete.zha

Comment 17

16 years ago
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

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

Comment 19

16 years ago
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+

Comment 20

16 years ago
Created attachment 89393 [details] [diff] [review]
patch (use next-in-flow)

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+

Comment 22

16 years ago
Created attachment 89500 [details] [diff] [review]
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.
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.)

Comment 25

16 years ago
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

Comment 26

16 years ago
Created attachment 111235 [details] [diff] [review]
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.
Attachment #89500 - Attachment is obsolete: true

Comment 27

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

Comment 28

16 years ago
Created attachment 111511 [details] [diff] [review]
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.

Updated

16 years ago
Attachment #111235 - Attachment is obsolete: true

Comment 29

16 years ago
Created attachment 111610 [details]
Testcase (Long links that wrap and image links which should all scroll into view)

Comment 30

15 years ago
Created attachment 123782 [details] [diff] [review]
Patch (uses GetNextInFlow() or GetNextSibling()) - v0.2

Patch against -current.
Attachment #111511 - Attachment is obsolete: true

Updated

15 years ago
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.

Comment 32

15 years ago
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?

Comment 34

15 years ago
Created attachment 125389 [details]
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?
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-

Comment 38

15 years ago
Created attachment 129729 [details] [diff] [review]
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().
Attachment #123782 - Attachment is obsolete: true

Updated

15 years ago
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...

Comment 40

15 years ago
You're very welcome to.

Reassigning.
Assignee: martin → dbaron

Comment 42

15 years ago
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
(Reporter)

Comment 45

12 years ago
See also bug 211854, same bug for textareas.
Created attachment 271290 [details] [diff] [review]
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.
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...

Comment 48

11 years ago
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 50

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9beta1
No.  I'll reland it once the tree opens.
You need to log in before you can comment on or make changes to this bug.