Closed Bug 940760 Opened 11 years ago Closed 10 years ago

Make layout/reftests/forms/input/number/focus-handling.html pass on Firefox OS

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Right now layout/reftests/forms/input/number/focus-handling.html fails on Firefox OS.

layout/reftests/forms/input/number/focus-handling.html | image comparison (==), max difference: 32, number of differing pixels: 1646

It looks like the :focus CSS styling is not being applied correctly.
<input type=number> is enabled on B2G, so we need to fix this for v28.
Understanding why the CSS :focus pseudo-class doesn't apply to <input type=number> needs some explanation of how <input type=number> is now implemented. It's frame, nsNumberControlFrame, creates a native anonymous content tree containing a div element wrapping a grandchild <input type=text> and some more div's for the spin buttons. It's the grandchild text control that takes focus when a user tries to focus the number control, so it's that grandchild that has the NS_EVENT_STATE_FOCUS bit set. For :focus to work on the <input type=number> (which is what b2g/chrome/content/content.css is using to try to apply the blue gradient background to the input to indicate focus) the NS_EVENT_STATE_FOCUS bit also needs to be set on the <input type=number>.
Attached patch patch (obsolete) — Splinter Review
Attachment #8343815 - Flags: review?(bugs)
RemoveStates/AddStates is really locked down (private methods). Rather than make HTMLInputElement/nsNumberControlFrame a friend of nsFocusManager/Element I've created a small class in nsFocusManager (for locality of focus handling) and locked it down to allow only nsNumberControlFrame to do a very specific thing (sync focus state).
Comment on attachment 8343815 [details] [diff] [review]
patch

I don't quite like this approach. I think we should make AddStates in Element
virtual and it should set the state to the correct element.
Attachment #8343815 - Flags: review?(bugs) → review-
That doesn't work if someone wants to hide the styling on the <input type=number>, and style the ::-moz-number-text with a focus style instead.
Attached patch patch (obsolete) — Splinter Review
That said getting :focus working for content is much more important.
Attachment #8343815 - Attachment is obsolete: true
Attachment #8344234 - Flags: review?(bugs)
Attached patch patch (obsolete) — Splinter Review
Attachment #8344234 - Attachment is obsolete: true
Attachment #8344234 - Flags: review?(bugs)
Attachment #8344235 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #5)
> I don't quite like this approach. I think we should make AddStates in Element
> virtual and it should set the state to the correct element.

This patch sets/removes the focus bits on both elements. If you insist we can have it set it only on the number control by simply returning after ownerNumberControl->Add/RemoveStates() calls.
For the record I'm not really a fan of this approach because it puts logic about what nsFocusManager::NotifyFocusStateChange does far away from that method making it more likely someone could change how NotifyFocusStateChange works without noticing to update HTMLInputElement code. It also seems bad to introduce virtual overhead to all Add/RemoveStates calls for this one special case. Anyway, up to you.
(In reply to Jonathan Watt [:jwatt] from comment #6)
> That doesn't work if someone wants to hide the styling on the <input
> type=number>, and style the ::-moz-number-text with a focus style instead.
technically the for the web page the input element should be focused, not some generated stuff.
(In reply to Jonathan Watt [:jwatt] from comment #10)
> For the record I'm not really a fan of this approach because it puts logic
> about what nsFocusManager::NotifyFocusStateChange does far away from that
> method making it more likely someone could change how NotifyFocusStateChange
> works without noticing to update HTMLInputElement code.
Well, your earlier approach added +nsFocusManager::NumberControlHelper which looked really odd, 
and for fixing type="file" etc one would need to have added similar classes for different
input types.

> It also seems bad to
> introduce virtual overhead to all Add/RemoveStates calls for this one
> special case. Anyway, up to you.
virtual method overhead is bad in hot code paths. This is not such case.
(In reply to Olli Pettay [:smaug] from comment #11)
> technically the for the web page the input element should be focused, not
> some generated stuff.

Yes, I was thinking of chrome style sheets that wants to change things.

(In reply to Olli Pettay [:smaug] from comment #12)
> Well, your earlier approach added +nsFocusManager::NumberControlHelper which
> looked really odd, 
> and for fixing type="file" etc one would need to have added similar classes
> for different
> input types.

We could change the name to FormControlHelpers, or something, and all we'd need to do to give file or other classes access would be to make them friends.

> virtual method overhead is bad in hot code paths. This is not such case.

Okay.
Comment on attachment 8344235 [details] [diff] [review]
patch

This looks much nicer.


>+++ b/content/base/public/Element.h
Update IID of Element


>+void
>+HTMLInputElement::AddStates(nsEventStates aStates)
>+{
>+  if (mType == NS_FORM_INPUT_TEXT) {
>+    bool onlyPassedFocusStateBits =
>+      (aStates->GetInternalValue() &
>+        !(NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING)) == 0;
This doesn't look right. I think you mean
bool onlyPassedFocusStateBits =
  (aStates->GetInternalValue() &
   ~(NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING)) == 0;


But I think something like the following would be simpler
nsEventStates focusStates(aStates->GetInternalValue() &
                          (NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING));
if (!focusStates.IsEmpty())
  HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
  if (ownerNumberControl) {
    ownerNumberControl->AddStates(focusStates);
  }
}
Attachment #8344235 - Flags: review?(bugs) → review-
Attached patch patchSplinter Review
(In reply to Olli Pettay [:smaug] from comment #14)
> This doesn't look right. I think you mean
> bool onlyPassedFocusStateBits =
>   (aStates->GetInternalValue() &
>    ~(NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING)) == 0;

Yikes! Yes, indeed.
Attachment #8344235 - Attachment is obsolete: true
Attachment #8344258 - Flags: review?(bugs)
Attachment #8344258 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/036c9a8c2025
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.