Closed
Bug 66619
Opened 24 years ago
Closed 17 years ago
tabbing to a multi-line link should try to make entire link visible
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P3)
Core
DOM: UI Events & Focus Handling
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)
15.34 KB,
text/html
|
Details | |
1.00 KB,
text/html
|
Details | |
6.57 KB,
patch
|
sharparrow1
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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•24 years ago
|
Status: NEW → ASSIGNED
OS: Windows 98 → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → Future
Updated•23 years ago
|
Comment 2•23 years ago
|
||
Reassigning alecf keyboard bugs. Chris, who should get this?
Assignee: alecf → saari
Status: ASSIGNED → NEW
Updated•23 years ago
|
Severity: enhancement → normal
Comment 3•23 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•23 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
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...
Comment 7•22 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.
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.
Just make all frames in same content visible when tab into the content.
Attachment #79609 -
Attachment description: popuse to fix → purpose to fix
Comment 10•22 years ago
|
||
Use original frame's height to increase frameBounds' height
Attachment #79609 -
Attachment is obsolete: true
Comment 11•22 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•22 years ago
|
||
Attachment #79786 -
Attachment is obsolete: true
Comment 13•22 years ago
|
||
Oops, this never got reviewed! Chris, could you r= this?
Comment 14•22 years ago
|
||
Comment on attachment 80935 [details] [diff] [review] use nextframe's height to increase frameBounds' height r=saari
Attachment #80935 -
Flags: review+
Comment 16•22 years ago
|
||
=>myself Sorry... I almost forget this bug :-P I will seeking sr= ASAP
Assignee: aaronl → pete.zha
Comment 17•22 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•22 years ago
|
||
cc'ing kin and dbaron, who can sanity check me here.
Comment 19•22 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•22 years ago
|
||
OK, change to use (next-in-flow) waterson, please sr= this one. Thanks!
Attachment #80935 -
Attachment is obsolete: true
Assignee | ||
Comment 21•22 years ago
|
||
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•22 years ago
|
||
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
Assignee | ||
Comment 23•22 years ago
|
||
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+
Assignee | ||
Comment 24•22 years ago
|
||
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•22 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•22 years ago
|
||
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•22 years ago
|
||
*** Bug 185163 has been marked as a duplicate of this bug. ***
Comment 28•22 years ago
|
||
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•22 years ago
|
Attachment #111235 -
Attachment is obsolete: true
Comment 29•22 years ago
|
||
Updated•21 years ago
|
Attachment #123782 -
Flags: review?(dbaron)
Assignee | ||
Comment 31•21 years ago
|
||
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•21 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.
Assignee | ||
Comment 33•21 years ago
|
||
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•21 years ago
|
||
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?
Assignee | ||
Comment 35•21 years ago
|
||
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.
Assignee | ||
Comment 36•21 years ago
|
||
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.
Assignee | ||
Comment 37•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
Attachment #129729 -
Flags: review?(dbaron)
Assignee | ||
Comment 39•21 years ago
|
||
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...
Assignee | ||
Comment 41•21 years ago
|
||
See also bug 195905.
Comment 42•20 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?
Assignee | ||
Comment 43•20 years ago
|
||
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.
Assignee | ||
Comment 44•20 years ago
|
||
...which is basically fixing bug 233164 correctly. See also bug 50511 comment 64.
Reporter | ||
Comment 45•18 years ago
|
||
See also bug 211854, same bug for textareas.
Assignee | ||
Comment 46•17 years ago
|
||
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)
Assignee | ||
Comment 47•17 years ago
|
||
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•17 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.
Assignee | ||
Comment 49•17 years ago
|
||
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•17 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+
Assignee | ||
Comment 51•17 years ago
|
||
I checked this in to the trunk earlier today; still need to file followup bugs.
Assignee | ||
Comment 52•17 years ago
|
||
I backed this out until I get review on bug 388019. I also have a mochitest for it on that bug.
Assignee | ||
Comment 54•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.9beta1
Comment 55•17 years ago
|
||
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?
Assignee | ||
Comment 56•17 years ago
|
||
No. I'll reland it once the tree opens.
Updated•5 years ago
|
Component: Keyboard: Navigation → User events and focus handling
Comment 57•5 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•