733379 - HTML input editor flags miss disabled flag in case of inherited disabled state

Status: NEW

(Core :: DOM: Editor, defect)

Description

[alexander :surkov (:asurkov)]

example is:

<fieldset id="f" disabled>
  <input id="f_input">
</fieldset>

input is disabled but
input.QueryInterface(nsIDOMNSEditableElement).editor;
editor.flags & nsIPlaintextEditor::eEditorDisabledMask == 0;

If input has disabled attribute then the flag is presented.

Comment 1

[Mounir Lamouri (:mounir)]

It seems that the code that handles changing this flag is in:
nsTextControlFrame::AttributeChanged
More exactly:
  else if (nsGkAtoms::disabled == aAttribute) 
  {
    PRUint32 flags;
    editor->GetFlags(&flags);
    if (AttributeExists(nsGkAtoms::disabled))
    { // set disabled
      flags |= nsIPlaintextEditor::eEditorDisabledMask;
      selCon->SetDisplaySelection(nsISelectionController::SELECTION_OFF);
      if (nsContentUtils::IsFocusedContent(mContent))
        selCon->SetCaretEnabled(false);
    }

Which indeed means that we are going to ignore the fieldset disabled attribute.

I think we should definitely make those editor flags being managed by the content instead of the layout. That way, we could easily handle that case and others (I think it's not the first bug open because of that).

Ehsan, does that sound good to you?

Comment 2

[(no longer active)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1)
> Ehsan, does that sound good to you?

Absolutely.  Previously the editor used to be attached to the frame, which is why this code is in layout currently.  Now, who's willing to knock up a patch for this?  :-)

Comment 3

[Mounir Lamouri (:mounir)]

In my opinion, we should add a callback for elements that is called when a state is changed. That way, we could simplify a lot of code in nsHTMLInputElement. For example, the editor and validation states could be updated when disabled/readonly states are changed.

It would be quite easy to implement but it might impact the performances because that would add a virtual call in UpdateState() when a state is changed. But can we consider a state change to be uncommon enough to take that perf regression?

Boris, what do you think of that?

Comment 4

[Boris Zbarsky [:bzbarsky]]

UpdateState already involves a virtual call and a bunch of computation, right?  And when state _did_ change the ContentStateChanged call.

I wouldn't have a huge problem with adding another hook in there in the "state actually changed" case, I think.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 6

[(no longer active)]

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

Review of attachment 612886 [details] [diff] [review]:
-----------------------------------------------------------------

r- for removing the @spellcheck check.  Also I'd like to look at the tests.

::: content/html/content/reftests/reftest.list
@@ +35,5 @@
> +#== href-attr-change-restyles.html href-attr-change-restyles-ref.html
> +#== figure.html figure-ref.html
> +
> +needs-focus == editor-disable-state-1.html editor-disable-state-1-ref.html
> +needs-focus == editor-disable-state-2.html editor-disable-state-2-ref.html

You forgot to include these tests in the patch.

::: content/html/content/test/forms/Makefile.in
@@ +64,5 @@
>  		test_maxlength_attribute.html \
>  		test_datalist_element.html \
>  		test_form_attributes_reflection.html \
>  		test_option_index_attribute.html \
> +		test_editor_disable_state.html \

This one too!  :-)

::: layout/forms/nsTextControlFrame.cpp
@@ +1239,2 @@
>    nsCOMPtr<nsIEditor> editor;
> +  if (nsGkAtoms::maxlength == aAttribute) {

You're removing the check for @spellcheck here.

Comment 7

[(no longer active)]

Also, why are you commenting out the tests in content/html/content/reftests/reftest.list?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Do we plan to use this callback for other things?  Because for what we're doing here just having the editor state be a mutation observer listening for ContentStateChanged seems like it would work just as well...

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Comment on attachment 612886 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 612886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for removing the @spellcheck check.  Also I'd like to look at the tests.

The "@spellcheck check" is to check if we need to get the editor but we never use the editor if @spellcheck has been changed so I don't see why we should get it... We actually don't do anything special if @spellcheck has been changed...

> ::: content/html/content/reftests/reftest.list
> @@ +35,5 @@
> > +#== href-attr-change-restyles.html href-attr-change-restyles-ref.html
> > +#== figure.html figure-ref.html
> > +
> > +needs-focus == editor-disable-state-1.html editor-disable-state-1-ref.html
> > +needs-focus == editor-disable-state-2.html editor-disable-state-2-ref.html
> 
> You forgot to include these tests in the patch.

Indeed.

> ::: content/html/content/test/forms/Makefile.in
> @@ +64,5 @@
> >  		test_maxlength_attribute.html \
> >  		test_datalist_element.html \
> >  		test_form_attributes_reflection.html \
> >  		test_option_index_attribute.html \
> > +		test_editor_disable_state.html \
> 
> This one too!  :-)

I actually forgot to remove the line here ;)

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 612886 [details] [diff] [review]
> Patch v1
> 
> Do we plan to use this callback for other things?  Because for what we're
> doing here just having the editor state be a mutation observer listening for
> ContentStateChanged seems like it would work just as well...

That would require to have nsHTMLInputElement being a DocumentObserver? Wouldn't that cost way more than adding this new callback?
I see one hypothetical other use case off hand but nothing else for the moment.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Comment 12

[(no longer active)]

Comment on attachment 614361 [details] [diff] [review]
Patch v1.1

Review of attachment 614361 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #6)
> > Comment on attachment 612886 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 612886 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- for removing the @spellcheck check.  Also I'd like to look at the tests.
> 
> The "@spellcheck check" is to check if we need to get the editor but we
> never use the editor if @spellcheck has been changed so I don't see why we
> should get it... We actually don't do anything special if @spellcheck has
> been changed...

OK, maybe GetEditor is not the best of names, but the purpose of that call is to make sure that the editor is initialized, because if the spellcheck attribute has been set, we need an actual editor object to perform the spellchecking.

::: content/html/content/reftests/reftest.list
@@ +35,5 @@
>  == href-attr-change-restyles.html href-attr-change-restyles-ref.html
>  == figure.html figure-ref.html
> +
> +needs-focus == editor-disable-state-1.html editor-disable-state-1-ref.html
> +needs-focus == editor-disable-state-2.html editor-disable-state-2-ref.html

Nit: please move these tests into layout/reftests/editor.

In addition to these tests, we need an inequality test to make sure that the appearance of <input value=foo> and <input value=foo disabled> is different.

Also, please add similar tests for textarea as well.

::: layout/forms/nsTextControlFrame.cpp
@@ -1241,5 @@
> -  nsISelectionController* selCon = txtCtrl->GetSelectionController();
> -  const bool needEditor = nsGkAtoms::maxlength == aAttribute ||
> -                            nsGkAtoms::readonly == aAttribute ||
> -                            nsGkAtoms::disabled == aAttribute ||
> -                            nsGkAtoms::spellcheck == aAttribute;

I think there is also one other bug introduced by this change that your tests are masking.

Let's say that you have an input which doesn't have a texteditor yet.  With this patch, when you set the disabled attribute on it, nsTextEditorState::ContentStateChanged gets called, but because the editor is not yet initialized, we bail out early.  Then, if you initialize the editor by doing something like focusing the element, the disabled editor flag will be lost, I think.

Thinking about this some more, I don't think we should remove any of these checks.

Also, we should have a test which does what I said above (note that such a test should fail with the current patch.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> That would require to have nsHTMLInputElement being a DocumentObserver?

Oh, state changes are on DocumentObserver?  Yeah, let's not do that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 614361 [details] [diff] [review]
Patch v1.1

sr=me

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.