Open Bug 733379 Opened 9 years ago Updated 8 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

ASSIGNED

People

(Reporter: surkov, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
Hardware: x86 → All
Version: unspecified → Trunk
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?
(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?  :-)
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?
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #612886 - Flags: review?(ehsan)
Attachment #612886 - Flags: superreview?(bzbarsky)
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.
Attachment #612886 - Flags: review?(ehsan) → review-
Also, why are you commenting out the tests in content/html/content/reftests/reftest.list?
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...
(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 ;)
(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.
Attached patch Patch v1.1Splinter Review
Attachment #612886 - Attachment is obsolete: true
Attachment #614361 - Flags: superreview?(bzbarsky)
Attachment #614361 - Flags: review?(ehsan)
Attachment #612886 - Flags: superreview?(bzbarsky)
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.)
Attachment #614361 - Flags: review?(ehsan) → review-
> That would require to have nsHTMLInputElement being a DocumentObserver?

Oh, state changes are on DocumentObserver?  Yeah, let's not do that.
Comment on attachment 614361 [details] [diff] [review]
Patch v1.1

sr=me
Attachment #614361 - Flags: superreview?(bzbarsky) → superreview+
You need to log in before you can comment on or make changes to this bug.