596088 - <input type='file'> doesn't look disabled when inside a disabled fieldest but is disabled

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mounir Lamouri (:mounir)]

We need to propagate the disabled state change to the anonymous content.

Even if it doesn't look disabled, it is.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Comment 2

[Mounir Lamouri (:mounir)]

How do content states work with anonymous children? It looks like they got the same state as their parents. Here, the button inside <input type='file'> correctly update its state but the text inside the button doesn't. If I call SetContentState() with any state on anonymous content, when the disabled state of the <input type='file'> changes, everything is fine.
Could that be a bug with content states updates?

Comment 3

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

Olli, can you answer comment #2?

Comment 4

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

Actually David has done more work with content states.

I need to read some code before answering...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> How do content states work with anonymous children?

Hmm...  They should Just Work, generally.

> It looks like they got the same state as their parents.

Er... no.  They don't.  They get an independent state.

It looks like in this case, the anonymous button is inheriting its border color from its ancestor (so it changes when you toggle the fieldset) but not actually changing its own state, right?  And specifically, that changing the <select>'s disabled state doesn't send a ContentStatesChanged notification for it or something?  Or does it?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> It looks like in this case, the anonymous button is inheriting its border color
> from its ancestor (so it changes when you toggle the fieldset) but not actually
> changing its own state, right?

Indeed.

> And specifically, that changing the <select>'s
> disabled state doesn't send a ContentStatesChanged notification for it or
> something?  Or does it?

Hmm, I don't follow. did you meant <legend> instead of <select>? In that case, yes, it should notify. Otherwise, I don't follow why you are speaking of <select>.

But I still don't understand why a call to SetContentStates() fix the issue with the button text color?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, I meant "fieldset", not "select".  I just checked in a debugger, and there is no ContentStatesChanged for the anon inputs when the select state is toggled.

If you call SetContentState() on the anon content what state are you passing to it?  Presumably ContentStatesChanged in that situation just ends up triggering a style reresolve...

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> Er, I meant "fieldset", not "select".  I just checked in a debugger, and there
> is no ContentStatesChanged for the anon inputs when the select state is
> toggled.

Indeed, ContentStatesChanged is called on the <input type='file'> inside the <fieldest> but not explicitely on anonymous content and I guess that might explain why SetContentState correctly update the style.

I'm wondering what would be the right way to fix this.
1. Can we consider that if the anonymous inputs have the correct style (disabled), it will be fine even if they are not really disabled? Given that no events can go through the <input type='file'> when disabled, I guess yes but I might be wrong.
2. Should we call ContentStatesChanged on anonymous children when ContentStatesChanged is called for an <input type='file'>? Should we make sure that the disabled state is correctly set in IntrinsicState() for the anonymous children? (related to 1.). This seem a bit ugly to me but I don't really know what is possible.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Can we consider that if the anonymous inputs have the correct style
> (disabled), it will be fine even if they are not really disabled?

Imo, yes.  Getting them to have the right style almost certainly involves them having the right intrinsic state.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> > Can we consider that if the anonymous inputs have the correct style
> > (disabled), it will be fine even if they are not really disabled?
> 
> Imo, yes.  Getting them to have the right style almost certainly involves them
> having the right intrinsic state.

As I said, I was able to get them to have the right style without the right intrinsic state.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Whether you can or not depends on your OS and theme, no?

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This seems to be easily fixable by introducing a ContentStatesChanged method to nsIFrame.
FWIW, I think this would be useful for other form controls (non-implemented HTML5 input types) at least.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 493844 [details] [diff] [review]
Patch v1

1)  Setting attrs inside ContentStatesChanged directly is unsafe.  It needs to
    happen off a script runner, at least.
2)  You need to change the IID of nsIFrame if you want to add methods to it. 
    And you can't do that on trunk for 2.0....
3)  There's no good reason for this to be NS_IMETHOD if/when we do add it to
    nsIFrame; just make it virtual void?
4)  Are we ok with the perf loss of an extra virtual call per
    ContentStatesChanged on anything with a frame?  I guess it shouldn't be
    too bad.

I'd like either roc or dbaron to sr the nsIFrame change, if we still want to make it.

Comment 14

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

(In reply to comment #13)
> 2)  You need to change the IID of nsIFrame if you want to add methods to it. 
>     And you can't do that on trunk for 2.0....

nsIFrame doesn't have an IID anymore, so this is a non-issue. yay!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Well, more to the point you can't change the vtable for 2.0, right?

Comment 16

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

I asked bsmedberg about this and he claimed that we can. See
https://bugzilla.mozilla.org/show_bug.cgi?id=581596#c78
and context.

Comment 17

[Boris Zbarsky [:bzbarsky]]

But we _know_ binary extensions use nsIFrame...  am I just missing something?

Comment 18

[Benjamin Smedberg]

I was under the impression that it was impossible for extensions to use nsIFrame any more, partly because we added a MOZILLA_INTERNAL_API guard somewhere.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

1. Fixed.
2. I saw there were no IID so I assumed it was fine.
3. Fixed.
4. Actually, I chose to do that instead of making nsFileControlFrame observing ContentStatesChanged because I will have to use this kind of behavior for other elements like <input type='number'> which will have a few anonymous content. If this seems to be a very specific need (IOW, if it sounds there is no other use case), I would understand to not add this virtual call. But, to me, with very small knowledge of the layout code, it doesn't sound useless to have this method in nsIFrame.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> partly because we added a MOZILLA_INTERNAL_API guard somewhere.

I don't see any such guards on nsIFrame or on the GetPrimaryFrame method of nsIContent...

So I'd bet money that things like roboform are still using nsIFrame.

Comment 21

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

Can we add such guards?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 493988 [details] [diff] [review]
Patch v2

>+   * This call is invoked when the content states of a content object's is
>+   * changed.
>+   * The first frame that maps that content is asked to deal with the change
>+   * by doing whatever is appropriate.

How about:

  When the content states of a content object change, this method is invoked on
  the primary frame of that content object.

r=me with that.

Comment 23

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/98ee88c41e23