Closed Bug 455891 Opened 16 years ago Closed 15 years ago

unable to set focus() on certain element types

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jmaher, Assigned: enndeakin)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
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.
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
Confirmed on fennec debug build pulled on 2008-09-22. Marking blocking as this prevents a significant number of mochitests from running.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-fennec1.0?
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).
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?
Attached patch something like this (obsolete) — Splinter Review
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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #345548 - Flags: review?(Olli.Pettay)
Attached patch mobile patch of patch (obsolete) — Splinter Review
I'll fix up the test as well if this looks OK.
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.
(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.
Blocks: 406837
Attachment #345548 - Flags: superreview?(roc)
Attachment #345548 - Flags: review?(roc)
Attachment #345548 - Flags: review?(Olli.Pettay)
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.
Attachment #345548 - Flags: superreview?(roc)
Attachment #345548 - Flags: superreview?(dbaron)
Attachment #345548 - Flags: review?(roc)
Attachment #345548 - Flags: review?(dbaron)
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.
(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.)
Blocks: 441582
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.
Attachment #345548 - Flags: superreview?(dbaron)
Attachment #345548 - Flags: superreview+
Attachment #345548 - Flags: review?(dbaron)
Attachment #345548 - Flags: review+
adding checkin-needed keyboard (?)
Keywords: checkin-needed
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
}
Attached patch unbitrotted patch (obsolete) — Splinter Review
unbitrotted neil's r+'ed and sr+'ed patch
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.
given mark's comments, patches are likely not ready to land as is in current tips.

unsetting checkin-needed
Keywords: checkin-needed
Blocks: 473558
Blocks: 473562
So what exactly still needs to happen here? If a deck isn't being used, is there a bug here?
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.
Attached patch make the browser offscreen (obsolete) — Splinter Review
OK, now that no deck is used, all that needs to happen here is to set the offscreen flag.
Attachment #359042 - Flags: review?(mark.finkle)
Comment on attachment 359042 [details] [diff] [review]
make the browser offscreen

actually, this should probably be set only for the active browser.
Attachment #359042 - Flags: review?(mark.finkle)
Attachment #345548 - Attachment is obsolete: true
Attachment #345549 - Attachment is obsolete: true
Attachment #355067 - Attachment is obsolete: true
Attachment #359042 - Attachment is obsolete: true
Attachment #359045 - Flags: review?(mark.finkle)
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.
Attachment #359045 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/906a16fa0b72
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: blocking-fennec1.0?
You need to log in before you can comment on or make changes to this bug.