User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041001 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041001 This comes from bug 257938. The need for using focus outlines to visually indicate to people who navigate via the keyboard is quite understood. However, there is no reason to have them at all when a navigating a Web site by click on elements / content areas. If the portion of the site has been selected via a mouse click, the user ALREADY knows where the focus it. There aren't any surprises. Further, having those outlines appear only servers to ruin the visual presentation of some sites that have been designed so as NOT to have lines surrounding their div:overflow elements. This is most noticeably a problem with div:overflow recently, but I've always found it irritating that it happened with other elements too. I've recently stumbled across setting "browser.display.focus_ring_width" to 0, which is wonderful, except that it doesn't work with div:overflow - I filed bug 262577 on that specific problem. Reproducible: Always Steps to Reproduce: 1. Go to the referenced URL. 2. Click on the main window. 3. Shift-Tab then Tab back and forth. Actual Results: Focus outline appears when clicking into the main window. It the disappears / reappears as you tab navigate to and fro it. Expected Results: Focus outline should only have appeared when tabbing to / from it. The initial mouse click should not have drawn the focus outline. I filed this under the same component as bug 257938 - feel free to move it as appropriate.

This is really hard to do, because the outline is just a :focus CSS rule. There's no way to make CSS dependent on how the focus was achieved.

I would be okay with a fix to this bug, but like I said it's not necessarily easy. We should take a look at how iframe handles this. That's the desired model, right?

I think IFRAME handles it like this: 1) IFRAMEs are focusable. 2) But when you click in an IFRAME, the IFRAME does not take focus. For the purposes of focusing on mousedown, IFRAMEs are not focusable. We could apply the same policy to overflow:auto/scroll elements. Clicking on such an element would not give the element focus, but it would focus or select content inside the element or even just place the (possibly invisible) caret inside the element. So you could still scroll the element with they keyboard. If that's acceptable to Section 508 then that sounds like a reasonable thing to do.

IFRAMEs are far from perfect, by the way. If you focus an IFRAME with the keyboard then the outline appears inside the IFRAME, which is wrong, and the outline disappears as soon as you do anything, e.g., press an arrow key to scroll, which is definitely wrong.

(In reply to comment #4) > IFRAMEs are far from perfect, by the way. If you focus an IFRAME with the > keyboard then the outline appears inside the IFRAME, which is wrong, and the > outline disappears as soon as you do anything, e.g., press an arrow key to > scroll, which is definitely wrong. I think they did those things on purpose. Iframe inherits that behavior from the root content pane (which I think is essentially an iframe). On the root content frame you really can't draw the focus outline outside the frame and you don't want dotted outlines hanging around all the time. Not that we can't do it differently for iframe's embedded in a page, but my 2 cents is that the dotted outlines look better inside the scrollbar containing elements.

(In reply to comment #5) > I think they did those things on purpose. Iframe inherits that behavior from > the root content pane (which I think is essentially an iframe). On the root > content frame you really can't draw the focus outline outside the frame and > you don't want dotted outlines hanging around all the time. > > Not that we can't do it differently for iframe's embedded in a page, but my 2 > cents is that the dotted outlines look better inside the scrollbar containing > elements. OK, let's compare these two proposals for in-page IFRAMEs: 1) Dotted outline inside the element, outline goes away as soon as you do anything 2) Dotted outline outside the element, outline persists as long as the element has focus I like 2) much more than 1). Surely 1) is not even Sec508 compliant, because a lot of the time keyboard users will have no visible indication of focus. And despite the fact that you think it looks better, I think 1) is less visually consistent with our other focus outlines. As for the root frame, I think the theme should give it enough of a border so that the focus outline can be shown outside the root frame. In Seamonkey Classic, this only requires the addition of a 1px border to the right edge.

Also, I wouldn't jump to conclusions about why things were done. When this stuff was coded up long ago, drawing the focus outline outside the element would have been hard. Even now fixing things is likely to be non-trivial.

(In reply to comment #7) > Also, I wouldn't jump to conclusions about why things were done. When this stuff > was coded up long ago, drawing the focus outline outside the element would have > been hard. Even now fixing things is likely to be non-trivial. I was there when people were talking about it. The problem is drawing a focus outline that isn't obscured by other parts of the window chrome.

So, if I understand this correctly, we really can't distinguish between keyboard navigation and mouse navigation with respect to CSS. Some part of Mozilla itself must know what event triggered the focus to an element - but that's not going to effect the CSS used in any way. Unless we have the event trigger a change in the stylesheet used (one, a keyboard navigation CSS, the other a mouse navigation CSS) - but, being quite ignorant of things, that may simply be a kludge that would introduce more problems, and performance hits, than anything else. So, if it must be the same for everyone (and the ability to set a preference to simply turn this off aside) I'm still not sure why we can't just outline the user input element itself. If, as is the case with a DIV that can be scrolled, rather than outlining the DIV we could outline its scrollbar. In other words, whatever piece of the UI has the ability for the user to do something - that's the piece that should be highlighted. Not the overall container. Actually, to be honest, this bug (unless it morphs) shouldn't be addressing possible alternatives to how outlining is currently displayed - that should happen somewhere else. If we can't, somehow, distinguish between keyboard and mouse navigation then we should resolve this as WONTFIX (too bad there isn't a CANTFIX) or, I suppose, just leave it open indefinitely.

erm, ignoring iframes, focus lasts longer than your average user's attention span, certainly longer than mine. if i click on something and mouse away so that it doesn't get the click event but is focussed. then i switch to another app, or walk away to get coffee. when i come back, i don't remember what i last clicked on. now there are all sorts of things i can do where i have no idea what will happen, unless there's a focus outline. because we have a focus outline, i know what will happen if i press enter.

Clicking on an element and then putting an outline around its scrollbars won't work. -- People will still complain about the look -- It's nonintuitive to click on an element and have something else get outlined -- What if there are two scrollbars? I still think we should do what I said in comment #3: > Clicking on such an element would not give the element focus, but it would > focus or select content inside the element or even just place the (possibly > invisible) caret inside the element. Fixing bugs in IFRAMEs and the main browser frame is a separable issue. Regarding timeless' comment, I don't see this as a problem. If I lose track of where the focus is, I just click somewhere so I know where the focus is and proceed. I don't look for a focus ring, fail to find one, and then collapse in terror.

I find the focus outline both beautiful and informative. I think is is important that mouse clicks also move focus to indicate that the clicked element is now the target for keyboard commands. Making keyboard scrolling separate (again) from focus only makes the UI more confusing. Users who don't like to have the focus outline drawn for certain elements can turn it off in their user stylesheets. (div:focus{-moz-outline:0;}) Regarding the main browser frame and (i)frames, I would like to see focus indicated also for those elements. Just my 2 cents...

Note that focus outlines on clicked links are invaluable -- they give you visual feedback on which link you clicked, and specifically that you HAVE clicked the link (especially for context-menu clicks and for sites that style active and normal links the same color).

One problem with this patch: after you click in an overflow:auto div with no scrollbars, arrow keys do nothing. Most users would probably assume page scrolling is broken, since they just see a document there, and don't know anything about the underlying CSS. Maybe GetViewToScroll should keep going up the parent view chain until there are scrollbars?

Well clearly I can't make everyone happy with this one. It really seems pretty minor in the whole scheme of things. The other solution I worked up for bug 257938. Boris, these div's aren't anchors. How much are you against this -- a number of people have asked for it.

> Boris, these div's aren't anchors. Yes, but I got the impression that we were globally turning off focus rings if things are clicked instead of tabbed to. After all, that's what the summary of this bug says. If I was wrong, great! > How much are you against this For the overflow case, I don't have an opinion either way, yet. I've not really encountered this in the wild, and haven't spent much time experimenting with it.

> One problem with this patch: after you click in an overflow:auto div > with no scrollbars, arrow keys do nothing. Why WOULD arrow keys do anything if there are no scrollbars? > How much are you against this -- a number of people have asked for it. I think it's pretty clear that we at the very least do need some conditional way of enabling or disabling this behaviour. I'm going to somewhat hesitantly mark bug 262577 as blocking this one.

(In reply to comment #16) > Created an attachment (id=161211) > aWithMouse as in param > > One problem with this patch: after you click in an overflow:auto div with no > scrollbars, arrow keys do nothing. Most users would probably assume page > scrolling is broken, since they just see a document there, and don't know > anything about the underlying CSS. Maybe GetViewToScroll should keep going up > the parent view chain until there are scrollbars? Yes. dbaron recently added the direction parameter so that GetViewToScroll would look for the nearest enclosing scrollable element whose scroll style was not HIDDEN in the relevant direction. This should be extended to look for an actual visible scrollbar and use the selection if there is no focus.

(In reply to comment #19) > Why WOULD arrow keys do anything if there are no scrollbars? I think I didn't explain well. At that point the arrow keys should be scrolling the main content where there are scrollbars, and there's no apparent reason (to the user) why they aren't. What's happening is that selection is in the overflow:auto div and Mozilla thinks that's where the arrow keys are being targeted.

Comment on attachment 161278 [details] [diff] [review] 1) Solves problem where arrow keys don't scroll anything, 2) Better algorithm in PresShell::GetViewToScroll() for finding scrolling view with correct aDirection Index: layout/base/public/nsIFrame.h + * @param [in, optional] aWithMouse, is focus is coming from mouse typo: "is focus is coming" Also, please update the comment at the beginning of GetNearestScrollingView(). The code seems correct for what you want to do, except for one (rather extreme) edge case - when the view is to narrow to fit the scrollbar, see testcase... I'm marking r- mostly to get a comment the edge case, but unless it's trivial to fix, I'm willing to convert that to a r+ on this patch.

Comment on attachment 161278 [details] [diff] [review] 1) Solves problem where arrow keys don't scroll anything, 2) Better algorithm in PresShell::GetViewToScroll() for finding scrolling view with correct aDirection >+ * GetRootFrameFor returns the root frame for a view >+ * @param aView is the view to return the root frame for >+ * @return the root frame for the view >+ */ >+ static nsIFrame* GetRootFrameFor(nsIView *aView); GetRootFrameFor is a bad name. It's not the root frame. It's just the frame for the view. Also, this should be inline.

(In reply to comment #23) > The code seems correct for what you want to do, except for one > (rather extreme) edge case - when the view is to narrow to fit the > scrollbar, see testcase... I think it's worth fixing this. I don't think we want to make such things completely unscrollable.

How do I test for a scrollable view where the scrollbars are hidden for lack of space?

With this patch we no longer need nsLayoutUtils::GetScrollableFrameFor(nsIScrollableView*) but I left it in since it's useful and doesn't really take up much more space.

Comment on attachment 161630 [details] [diff] [review] Works even when there's simply no room to display the scrollbars. New algroithm knows if there's stuff to scroll by comparing the total area in the scrollable container to the visible frame size. nsEventStateManager.cpp:1011: error: `ChangeFocusWith' undeclared (first use this function) You must have ns(I)EventStateManager.h changes too?

The page should not scroll when the blue box is focused?

In this case there are scrollbars but nowhere to scroll. It's kind of the opposite of the case where there are no scrollbars but you can scroll. Anyway, I don't see this case as a big deal. If you want I'll add more complexity to the logic to handle this case, but I'm not really sure that it matters or is worth it.

(In reply to comment #32) > Anyway, I don't see this case as a big deal. I would much rather have this fixed than the first edge case because I'm guessing this content is fairly common (and the first edge case isn't). When I combine your patches, both testcases works as expected: if (aDirection != eHorizontal && ss.mVertical != NS_STYLE_OVERFLOW_HIDDEN && (totalHeight > visibleSize.height || margin.right)) break; if (aDirection != eVertical && ss.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN && (totalWidth > visibleSize.width || margin.bottom)) break; }

Comment on attachment 161641 [details] [diff] [review] Combine the logic to get all edge cases This looks good, but I've found a case where the assertion fails and we crash: #7 0x41dc72bf in nsLayoutUtils::GetNearestScrollingView(nsIView*, nsLayoutUtils::Direction) (aView=0x870d230, aDirection=eEither) at nsLayoutUtils.cpp:392 #8 0x41e5f537 in nsTypedSelection::ScrollPointIntoView(nsPresContext*, nsIView*, nsPoint&, int, int*) (this=0x87395a0, aPresContext=0x8737b20, aView=0xbfffd890, aPoint=@0xbfffda00, aScrollParentViews=1, aDidScroll=0xbfffd8ec) at nsSelection.cpp:5455 #9 0x41e5f6a4 in nsTypedSelection::DoAutoScrollView(nsPresContext*, nsIView*, nsPoint&, int) (this=0x87395a0, aPresContext=0x8737b20, aView=0x8763958, aPoint=@0xbfffda00, aScrollParentViews=1) at nsSelection.cpp:5541 #10 0x41e5f0ec in nsTypedSelection::StartAutoScrollTimer(nsPresContext*, nsIView*, nsPoint&, unsigned) (this=0x87395a0, .... (gdb) fr 7 #7 0x41dc72bf in nsLayoutUtils::GetNearestScrollingView(nsIView*, nsLayoutUtils::Direction) (aView=0x870d230, aDirection=eEither) at nsLayoutUtils.cpp:392 392 nsMargin margin = scrollableFrame->GetActualScrollbarSizes(); (gdb) p scrollableFrame $1 = (class nsIScrollableFrame *) 0x0 STEPS TO REPRODUCE: 1. load "Testcase - complex 1 in FRAMEs" from bug 97283 (attachment 153478 [details]) 2. drag-select to the right of the green block, on the upper FRAME background. This seems like an unrelated bug - maybe we can just add if (!scrollableFrame) break; after the assertion and spawn it off as a separate bug?

mats, what you're seeing is that we don't create scrollframes for framesets. That's a bug. We have other places in layout asserting on this already.... See discussion in bug 260652.

Okay, I can put the null check in. Is it necessary to post a new patch for that? Or could we just use the current patch for review contingent on the null check?

Comment on attachment 161641 [details] [diff] [review] Combine the logic to get all edge cases r+ with the added null check (could you add an XXX comment referring this bug too?)

Comment on attachment 161278 [details] [diff] [review] 1) Solves problem where arrow keys don't scroll anything, 2) Better algorithm in PresShell::GetViewToScroll() for finding scrolling view with correct aDirection This patch is old. There is a newer patch posted with r+=mats, sr?=roc

Comment on attachment 161641 [details] [diff] [review] Combine the logic to get all edge cases Yeah, this is what I wanted :-)

Checking in layout/base/public/nsIFrame.h; /cvsroot/mozilla/layout/base/public/nsIFrame.h,v <-- nsIFrame.h new revision: 3.262; previous revision: 3.261 done Checking in layout/base/public/nsLayoutUtils.h; /cvsroot/mozilla/layout/base/public/nsLayoutUtils.h,v <-- nsLayoutUtils.h new revision: 3.16; previous revision: 3.15 done Checking in layout/html/base/src/nsFrame.cpp; /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v <-- nsFrame.cpp new revision: 3.522; previous revision: 3.521 done Checking in layout/html/base/src/nsPresShell.cpp; /cvsroot/mozilla/layout/html/base/src/nsPresShell.cpp,v <-- nsPresShell.cpp new revision: 3.785; previous revision: 3.784 done Checking in layout/base/src/nsLayoutUtils.cpp; /cvsroot/mozilla/layout/base/src/nsLayoutUtils.cpp,v <-- nsLayoutUtils.cpp new revision: 3.22; previous revision: 3.21 done Checking in content/events/src/nsEventStateManager.cpp; /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v <-- nsEventStateManager.cpp new revision: 1.538; previous revision: 1.537 done

Comment on attachment 161641 [details] [diff] [review] Combine the logic to get all edge cases >+ // If this very frame provides a scroll view, start there instead of frame's >+ // closest view, because the scroll view may be inside a child frame. >+ // For example, this happens in the case of overflow:scroll. >+ // In that case we still use GetNearestScrollingView() because >+ // we need a scrolling view that matches aDirection. >+ nsIView* startView = svp? svp->GetScrollableView()->View() : startFrame->GetClosestView(); >+ NS_ASSERTION(startView, "No view to start searching for scrollable view from"); Unfortunately nsIScrollableView::View is not null-safe, and you don't null-check svp->GetScrollableView(), and I managed to crash here.

Unfortunately I wasn't able to subsequently reproduce my crash, but for your information the steps that originally caused the crash were as follows: 1. Press F9 to reveal the sidebar 2. Tab to the [Tabs v] button 3. Press down arrow

Why reopen this bug? If this code is causing problems now (and this is the first report in almost 3 months of a problem), shouldn't a new bug should be filed?

Why reopen this bug? If this code is causing problems now (and this is the first report in almost 3 months of a problem), shouldn't a new bug be filed?

Does this mean bug 262578 can be reclosed now?

Does the fix to bug 278546 mean this can be closed now?

Yes, this is fixed...

(In reply to comment #45) >Why reopen this bug? If this code is causing problems now (and this is the >first report in almost 3 months of a problem), shouldn't a new bug be filed? Sorry for not keeping this bug up-to-date. In fact, I had even forgotton that I had reopened it :-[ For your information, the circumstances of the bug occur quite rarely which explains why nobody was able to reliably reproduce the crash.