455891 - unable to set focus() on certain element types

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080916182923 Fennec/1.0a1pre

Mochitests has good coverage of various elements and testing for setting focus on those elements.  Currently out of 680 tests, 241 fail.  I have created a very simple .html file to demonstrate one of the common conditions failing.  Hopefully the fix to make this simple file pass, will fix the majority if not all of the 241 mochitests which fail only on fennec.

I have a <div> with <a tabindex="-1"></a> inside of it.  I cannot set the focus on the <a> tag.  

Reproducible: Always

Steps to Reproduce:
1. launch fennec with fail241.html (attachment)
2. observe the alert that pops up
3. verify in Fx3.1 that the alert does not popup
Actual Results:  
focus is not set on the <a> tag.

Expected Results:  
We are able to set the focus with the focus() function and query the status with .activeElement.

Please see the attached fail241.html file for an example and any clarification on what is failing.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: Test case to demonstrate bug functionality

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: another test case showing focus on the wrong

There is another mochitest suite which has 5 failures: /tests/content/base/test/test_bug337631.html which appears to have the same problem with focus.

I have created another file fail5.html which shows fennec getting the wrong element when focus() is called.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I ran into this again while investigating the fennec failures.  It appears that the focus in fennec is always the "body" and not the element.

This case has 6 failures all happen after the "focus()" call is made:
/tests/content/html/content/test/test_bug300691-1.html

Comment 4

[cmtalbert]

Confirmed on fennec debug build pulled on 2008-09-22. Marking blocking as this prevents a significant number of mochitests from running.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I suppose this might have to do with not being able to focus something that isn't visible (since the actual browser is hidden and redrawn to the canvas).

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

this bug has been idle for 3 weeks.  As discussed in the Monday meeting, can we get some traction on this bug before end of week?

Comment 7

[Neil Deakin]

Attachment: something like this

This fixes the first testcase and somewhat fixes the second, without regressing bug 454235 (I think). Instead of marking the docshell as offscreen, an attribute is added to the children of <deck> elements.

Comment 8

[Neil Deakin]

Attachment: mobile patch of patch

Comment 9

[Neil Deakin]

I'll fix up the test as well if this looks OK.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 345548 [details] [diff] [review]
something like this

I'm not sure I'm the right person to review this.
I don't like making a method called AreAncestorViewsVisible() to return PR_TRUE, even if some of the views are hidden.
Docshell::GetVisibility is a bit different. The method name doesn't say anything about views.

Comment 11

[Neil Deakin]

(In reply to comment #10)
> (From update of attachment 345548 [details] [diff] [review])
> I'm not sure I'm the right person to review this.
> I don't like making a method called AreAncestorViewsVisible() to return
> PR_TRUE, even if some of the views are hidden.
> Docshell::GetVisibility is a bit different. The method name doesn't say
> anything about views.

OK. I was actually thinking of changing the method name to AreAncestorViewsVisibleForFocus or something like that.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 345548 [details] [diff] [review]
something like this

I'd actually like dbaron to review this since he's more or less in charge of the "hidden docshell" stuff.

Comment 13

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

Boy, AreAncestorViewsVisible sure wasn't meant to be used for things like focus; it was only intended to be good enough to say that things can be optimized away, not reliable enough that it guarantees something is shown.  Then again, I supposes it fixes the tabbrowser case for focus.

We should really rip AreAncestorViewsVisible out, have tabbrowser set something on the docshell for all the tabs that aren't shown to tell that docshell that layout-ish stuff can be optimized away, and have the focus tests call into the appropriate docshell (rather than having docshell call into layout).

The question is whether I should make you do that in order to fix this bug rather than piling another hack on top of the existing mess... let me think about that for a bit.

Comment 14

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

(And note that I think the idea here would be a new method on docshell, although it's possible there's an existing one that doesn't do anything useful.  If an existing one does do something useful, probably better to keep things separate for simplicity.)

Comment 15

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

Comment on attachment 345548 [details] [diff] [review]
something like this

OK, I filed the separate bug (bug 465216) on the real fix here, so r+sr=dbaron for this temporary one.

Comment 16

[Antonio Gomes (tonikitoo)]

adding checkin-needed keyboard (?)

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 345548 [details] [diff] [review]
something like this

Patch has bit-rotted:
{
patching file docshell/base/nsDocShell.cpp
Hunk #1 FAILED at 295
Hunk #2 FAILED at 3942
Hunk #3 FAILED at 3968
3 out of 4 hunks FAILED
}

Comment 18

[Antonio Gomes (tonikitoo)]

Attachment: unbitrotted patch

unbitrotted neil's r+'ed and sr+'ed patch

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Mobile trunk no longer puts the <browser>s in a <deck>, because <deck> created a native widget and slowed things down.

However, a fix recently landed that stops <deck> from creating a native widget, for mobile only (I don't like that aspect, but anyway)

The "mobile patch of patch" will need to be updated since it is bitrotted too. The updated patch will need to put the new list of <browser>s in a <deck> instead of a <vbox>.

OR the main patch here will need to stop assuming the "hiddenfocus" elements live in a <deck> to begin with.

I kinda like the latter approach. What's so special about <deck>? Making this <deck> specific seems like more of a hack.

Comment 20

[Antonio Gomes (tonikitoo)]

given mark's comments, patches are likely not ready to land as is in current tips.

unsetting checkin-needed

Comment 21

[Neil Deakin]

So what exactly still needs to happen here? If a deck isn't being used, is there a bug here?

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

we still have a lot of tests failing which appear to be related to focus.  I haven't tried the unbitrotted patch from Jan 1.  I will work on trying that out.

Comment 23

[Neil Deakin]

Attachment: make the browser offscreen

OK, now that no deck is used, all that needs to happen here is to set the offscreen flag.

Comment 24

[Neil Deakin]

Comment on attachment 359042 [details] [diff] [review]
make the browser offscreen

actually, this should probably be set only for the active browser.

Comment 25

[Neil Deakin]

Attachment: update offscreen state as needed

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 359045 [details] [diff] [review]
update offscreen state as needed

I'm assuming that thee default is | false | so we don't need to set isOffScreenBrowser when we create the <browser>s.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/906a16fa0b72

Comment 29

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090821
Fennec/1.0b3pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre