268497 - keyboard scrolling ends up in select widget when it shouldn't

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is the regression bug that I've mentioned a few times but never filed --
probably related to bug 97283 or the corresponding keyboard bug.

Steps to reproduce:
 1. load https://bugzilla.mozilla.org/query.cgi
 2. make the window wide enough so that you can see past the right edge of
    the target combobox
 3. click in the "Component" dropdown (so you select "Address Book" or
    something like that)
 4. click in the blank space to the right of the "Target" combobox
 5. press the down arrow

Expected results:
 5. page scrolls down

Actual results:
 5. "Target" combobox scrolls down

Firefox Linux trunk, November 3.

Comment 1

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041109

no need to select anything, all boxes having a scrollbar are showing this
behaviour, i.e. Product, Component, Version, Target, Hardware, OS.
Exception in Bug Changes, the box 'where one or more of the following changed:'
doesn´t show this behaviour.

1. load https://bugzilla.mozilla.org/query.cgi
2. click to the left or the right of any one of the boxes mentioned above
3. use key to scroll

Area below the boxes is also sensitive:

4. click in the gap above the 'A Comment' boxes, below the Product, Component,
Version, Target boxes: keys are scrolling the contents of the box above

5. click in the gap above the 'Bug Changes' box, below the Hardware and OS boxes: 
clicked few pixels below Hardware/OS box: keys are scrolling the contents of the
box above
Clicked some more pixels down: keys are scrolling the page.

Comment 2

[Hermann Schwab]

Attachment: reduced testcase #1

click left, right or below one of the upper boxes, and keys scroll the content
of the box.

Comment 3

[Hermann Schwab]

Attachment: all sides testcase

click above upper hr, or below lower hr, and scrolling keys don´t work.
click below upper hr, or above lower hr, and scrolling keys scroll contents of
a box.
click above upper hr, or below lower hr, and scrolling keys are working on the
box they´ve worked before. Click anywhere above upper hr or anywhere below
lower hr seems to not happen, doesn´t affect the  behaviour of the scrolling
keys.
click right, left, above or below, one of the boxes, and keys are scrolling the
contents.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Nominating for blocking1.8a6 -- it's a regression from changes in the 1.8 cycle,
and I don't think we should ship it in 1.8final.

Comment 5

[Asa Dotzler [:asa]]

who can help here? aaronl, are can you look at this? 

Do we know what regressed this? or when? If not, Tracy, can you get a regression
window for us?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm pretty sure it was bug 97283 or bug 251986 (more likely the latter, I
think).  Both those bugs involved new code that just needs to be tweaked to fix
this.

Comment 7

[Aaron Leventhal]

I can't look at this right now. Sorry.

Comment 8

[Tracy Walker [:tracy]]

The regression window is much more recent than when bug 97283 or bug 251986 were
marked fixed.  The tescase worked up until 1.8a4 and is broken in 1.8a5.  I'll
narrow it down further.

Comment 9

[Hermann Schwab]

wfm BuildId 2004101806, failing BuildID 2004102004

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-18+02%3A00&maxdate=2004-10-20+04%3A00&cvsroot=%2Fcvsroot

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

2004-10-18 19:24	aaronleventhal%moonset.net 	mozilla/ content/ events/ src/
nsEventStateManager.cpp 	1.538 	2/1  	Bug 262578. No click to focus css
scrollable areas like overflow:scroll, but you can still click there and scroll
-- just no focus outline. r=mats, sr=roc

Comment 10

[Boris Zbarsky [:bzbarsky]]

The steps in comment 0 show the bug in build 2004-10-19-07 but not in
2004-10-18-08.  The one checkin in that range that looks remotely related is bug
262578.

I'm not sure whether the issue with the "where one or more of the following
changed:" select on the same page is this same bug, but it shows up in older
builds (and I was sure I'd seen someone comment on why it was happening... it
was either Mats or Neil...)

Comment 11

[[e-mail]]

The reason that the effect does not appear to occur with the "where one or more
of the following changed:" select appears to be because the "Only bugs changed
between:" input is deemed to be scrollable. For a couple of minimal testcases,
try data:text/html,<textarea> and data:text/html,<input><textarea>

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase(s)

I can see most of these problems in all releases back to and including 1.0.2.
I have a fix for it in my tree - unfortunately it breaks "browse-with-caret"
into a list control in the sense that you can't navigate in to it from outside.

I'm trying to figure out if I can fix that (it works for textarea, so there
must be something special with it (it has its own selection controller
maybe?)).
Another route would be to check for "browse-with-caret" - can anyone tell me
how I can test that, given a frame?

Comment 13

[Mats Palmgren (inactive)]

(In reply to comment #12)
> (it works for textarea, so there must be something special ..

The difference is that the leaf text nodes of a textarea is
"IsSelectable()" whereas inside a list control they are not.
This is what "nsFrame::GetFrameFromDirection()" looks at.

If I make them selectable in the UA stylesheet I can browse-
with-caret into the list control, but this also opens up
selection in normal mode which we don't want, so a solution
might be to add some rules to the pref. sheet only when
"browse-with-caret" is on...

Comment 14

[Mats Palmgren (inactive)]

Comment on attachment 170533 [details]
Testcase(s)

I should have said that in Test 1,2,3,4 the
form control should not scroll.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes all the mentioned problems in "normal" mode. It does not change
"browse-with-caret" mode at all.

(Removing the condition on the "browsewithcaret" pref also fixes some selection
problems in this mode, but this regressed navigation somewhat (the x-position
was not maintained when navigating UP/DOWN and you had to press RIGHT to get
into the list/textarea - so I decided to leave "browsewithcaret" as is.
Let me know if you're interested in the code)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some of the other code that uses NS_FRAME_INDEPENDENT_SELECTION (in particular,
nsFrame::GetSelectionController) seems to assume that there's a text control
frame involved when that bit is set.

Comment 17

[Mats Palmgren (inactive)]

(In reply to comment #16)
> Some of the other code that uses NS_FRAME_INDEPENDENT_SELECTION (in
> particular,
> nsFrame::GetSelectionController) seems to assume that there's a text control
> frame involved when that bit is set.

OK, I will take a closer look on how the bit is currently used.
As for nsFrame::GetSelectionController, this seems mostly a performance
issue, we should stop the loop when we find a frame without the
INDEPENDENT_SELECTION bit and then it won't be a problem.

I do see now that this bit is inherited:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrame.cpp&rev=3.537&root=/cvsroot&mark=562-568#539
Oops, I didn't notice that before so my patch has to account for that...

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

This is more what I intended the first time - it's only crossing the boundary
into an INDEPENDENT_SELECTION frame tree we want to avoid.

I also fixed the performance issue with nsFrame::GetSelectionController()
when it's not a text control.

The other (2) uses should be OK (BlockFrame and TypeAheadFind):
http://lxr.mozilla.org/seamonkey/search?string=INDEPENDENT_SELECTION

Comment 19

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this until at least Sunday...

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 170585 [details] [diff] [review]
Patch rev. 2

sr=dbaron, although this still scares me a bit because of changing the meaning
of NS_FRAME_INDEPENDENT_SELECTION.

I think it's probably better to wait until after a6 to land this -- now that we
have a patch I don't think there's much risk of it missing 1.8final, which was
the main concern.

Comment 21

[Asa Dotzler [:asa]]

minusing per dbaron's comment.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170585 [details] [diff] [review]
Patch rev. 2

r=bzbarsky, but please test things like typeahead find (which uses this bit),
clicks on listboxes and comboboxes (which seem to look at this stuff, via
blockframe), and keep an eye on regressions....

Comment 23

[Mats Palmgren (inactive)]

Checked in 2005-01-16 10:44 PDT

-> FIXED

Comment 24

[Simon Fraser [no longer active]]

It looks like this might be causing pageload crashes on tinderbox.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, that's probably just the intermittent problems with axolotl that we've been
having.