Focus outlines on css scrollable areas should only apply to keyboard based navigation.

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: jasonb, Assigned: aaronlev)

Tracking

(Depends on: 1 bug, {crash, helpwanted})

Trunk
x86
Windows XP
crash, helpwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Waiting for final go ahead from roc, URL)

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
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.
(Assignee)

Comment 2

14 years ago
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?
Keywords: helpwanted
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.
(Assignee)

Comment 5

14 years ago
(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.
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

14 years ago
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.
(Assignee)

Comment 12

14 years ago
Created attachment 161157 [details] [diff] [review]
CSS scrollable items take selection on click, but not focus. Allow GetViewToScroll to use selection.
(Assignee)

Updated

14 years ago
Attachment #161157 - Flags: superreview?(roc)
(Assignee)

Updated

14 years ago
Attachment #161157 - Flags: review?(mats.palmgren)
(Assignee)

Updated

14 years ago
Attachment #161157 - Flags: superreview?(roc)
Attachment #161157 - Flags: review?(mats.palmgren)
(Assignee)

Comment 13

14 years ago
Created attachment 161185 [details] [diff] [review]
Add aWithMouse parameter to clarify code
Attachment #161157 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161185 - Flags: superreview?(roc)
Attachment #161185 - Flags: review?(mats.palmgren)
(Assignee)

Updated

14 years ago
Attachment #161185 - Flags: superreview?(roc)
Attachment #161185 - Flags: review?(mats.palmgren)
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).
(Assignee)

Comment 16

14 years ago
Created attachment 161211 [details] [diff] [review]
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?
Attachment #161185 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161211 - Flags: superreview?(roc)
(Assignee)

Comment 17

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

Comment 19

14 years ago
> 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.
Depends on: 262577
(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.
(Assignee)

Comment 21

14 years ago
(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.
(Assignee)

Updated

14 years ago
Attachment #161211 - Flags: superreview?(roc)
(Assignee)

Comment 22

14 years ago
Created 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
Attachment #161211 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161278 - Flags: superreview?(roc)
(Assignee)

Updated

14 years ago
Summary: Focus outlines should only apply to keyboard based navigation. → Focus outlines on css scrollable areas should only apply to keyboard based navigation.
(Assignee)

Updated

14 years ago
Attachment #161278 - Flags: review?(mats.palmgren)
(Assignee)

Updated

14 years ago
Whiteboard: Waiting for final go ahead from roc
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.
Attachment #161278 - Flags: review?(mats.palmgren) → review-
Created attachment 161619 [details]
Testcase (edge case)
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.
(Assignee)

Comment 27

14 years ago
How do I test for a scrollable view where the scrollbars are hidden for lack of
space?
(Assignee)

Comment 28

14 years ago
Created 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.

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.
Attachment #161278 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161630 - Flags: review?(mats.palmgren)
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?
Attachment #161630 - Flags: review?(mats.palmgren) → review-
(Assignee)

Comment 30

14 years ago
Created attachment 161634 [details] [diff] [review]
Oops, sorry. That's from a different patch. Here's a clean one.
Attachment #161630 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161634 - Flags: review?(mats.palmgren)
Created attachment 161639 [details]
Testcase #2

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

Comment 32

14 years ago
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;
    }
(Assignee)

Comment 34

14 years ago
Created attachment 161641 [details] [diff] [review]
Combine the logic to get all edge cases
Attachment #161634 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #161634 - Flags: review?(mats.palmgren)
(Assignee)

Updated

14 years ago
Attachment #161641 - Flags: review?(mats.palmgren)
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?
Attachment #161641 - Flags: review?(mats.palmgren) → review-
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.
(Assignee)

Comment 37

14 years ago
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?)
Attachment #161641 - Flags: review- → review+
(Assignee)

Updated

14 years ago
Attachment #161641 - Flags: superreview?(roc)
(Assignee)

Comment 39

14 years ago
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
Attachment #161278 - Flags: superreview?(roc)
Comment on attachment 161641 [details] [diff] [review]
Combine the logic to get all edge cases

Yeah, this is what I wanted :-)
Attachment #161641 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 41

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Reporter)

Updated

14 years ago
Status: RESOLVED → VERIFIED
Depends on: 265940

Comment 42

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

Comment 43

14 years ago
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.8b?
Keywords: crash
(Reporter)

Comment 44

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

Comment 45

14 years ago
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?
(Assignee)

Comment 46

14 years ago
Does this mean bug 262578 can be reclosed now?
(Assignee)

Comment 47

14 years ago
Does the fix to bug 278546 mean this can be closed now?
Yes, this is fixed...
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 49

14 years ago
(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.

Updated

14 years ago
Flags: blocking1.8b?
You need to log in before you can comment on or make changes to this bug.