596100 - HTMLInputElement should be changing the editor state instead of nsTextControlFrame

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Mounir Lamouri (:mounir)]

The editor is now owned by the input element but the frame is still changing some modes linked to attributes.

I have to change that for bug 557087 so I might change for all kind of editor states linked to content attributes.

Ehsan, let me know if that doesn't seem as easy as it looks.

Comment 1

[(no longer active)]

I'm not really sure what you're trying to do here.  Can you be more specific, please?

Comment 2

[Mounir Lamouri (:mounir)]

nsTextControlFrame is changing the states of the editor when an attribute is changed (in nsTextControlFrame::AttributeChanged).
For example, this is called when the disabled state is set:
    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(PR_FALSE);
    }

It seems better to have this done by the content given that the content now owns the editor and is more appropriate for attribute changes actions.

Comment 3

[Mounir Lamouri (:mounir)]

BTW, I've open this bug because text controls can know be disabled like this:
<fieldset disabled>
  <input>
</fieldset>

In that case, the editor inside the input will be enabled. Hopefully, there are no real consequences given that the user can't interact with the element.

Comment 4

[(no longer active)]

(In reply to comment #2)
> nsTextControlFrame is changing the states of the editor when an attribute is
> changed (in nsTextControlFrame::AttributeChanged).
> For example, this is called when the disabled state is set:
>     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(PR_FALSE);
>     }
> 
> It seems better to have this done by the content given that the content now
> owns the editor and is more appropriate for attribute changes actions.

Ah, OK.  Yes, moving those pieces of code to content should be safe, as long as you handle the case where the operation requires a frame to be bound to the content node, and the content node does not have a frame.

Comment 5

[(no longer active)]

(In reply to comment #3)
> BTW, I've open this bug because text controls can know be disabled like this:
> <fieldset disabled>
>   <input>
> </fieldset>
> 
> In that case, the editor inside the input will be enabled. Hopefully, there are
> no real consequences given that the user can't interact with the element.

What types of consequences are you worried about here?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> (In reply to comment #3)
> > BTW, I've open this bug because text controls can know be disabled like this:
> > <fieldset disabled>
> >   <input>
> > </fieldset>
> > 
> > In that case, the editor inside the input will be enabled. Hopefully, there are
> > no real consequences given that the user can't interact with the element.
> 
> What types of consequences are you worried about here?

None actually now. I was worried about that before (when I saw the frame was enabling/disabling the editor depending of the disabled attribute) but an enabled editor inside a disabled text field doesn't seem to have any consequence.
It would be better to move that to the content though.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > BTW, I've open this bug because text controls can know be disabled like this:
> > > <fieldset disabled>
> > >   <input>
> > > </fieldset>
> > > 
> > > In that case, the editor inside the input will be enabled. Hopefully, there are
> > > no real consequences given that the user can't interact with the element.
> > 
> > What types of consequences are you worried about here?
> 
> None actually now. I was worried about that before (when I saw the frame was
> enabling/disabling the editor depending of the disabled attribute) but an
> enabled editor inside a disabled text field doesn't seem to have any
> consequence.
> It would be better to move that to the content though.

Wait, that shouldn't happen.  Setting the content's disabled attribute should set the disabled flag on the editor, doesn't it?

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> Wait, that shouldn't happen.  Setting the content's disabled attribute should
> set the disabled flag on the editor, doesn't it?

As I said in comment 3, the element can now be disabled because of its fieldset so the disabled attribute might not be set.

Comment 9

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #7)
> > Wait, that shouldn't happen.  Setting the content's disabled attribute should
> > set the disabled flag on the editor, doesn't it?
> 
> As I said in comment 3, the element can now be disabled because of its fieldset
> so the disabled attribute might not be set.

I see.  Yes.  That's the sort of problem that we can solve if we move the handling of the disabled attribute to the content node.  I think relying on the fact that things work _now_ is fragile.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> I see.  Yes.  That's the sort of problem that we can solve if we move the
> handling of the disabled attribute to the content node.  I think relying on the
> fact that things work _now_ is fragile.

Do you think it worths being fixed for 2.0? (to know if I move that to my TODO list)

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #9)
> > I see.  Yes.  That's the sort of problem that we can solve if we move the
> > handling of the disabled attribute to the content node.  I think relying on the
> > fact that things work _now_ is fragile.
> 
> Do you think it worths being fixed for 2.0? (to know if I move that to my TODO
> list)

Hmm, not really, unless there's a web compat bug or a regression which would be fixed by this.  This is kind of risky business.  I fixed the last regression from moving editor to content just last week.  ;-)

Comment 12

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I see.  Yes.  That's the sort of problem that we can solve if we move the
> > > handling of the disabled attribute to the content node.  I think relying on the
> > > fact that things work _now_ is fragile.
> > 
> > Do you think it worths being fixed for 2.0? (to know if I move that to my TODO
> > list)
> 
> Hmm, not really, unless there's a web compat bug or a regression which would
> be fixed by this.  This is kind of risky business.  I fixed the last
> regression from moving editor to content just last week.  ;-)

Make that yesterday!  ;)

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.