HTMLInputElement should be changing the editor state instead of nsTextControlFrame

NEW
Unassigned

Status

()

Core
DOM: Core & HTML
8 years ago
7 years ago

People

(Reporter: mounir, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
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.
I'm not really sure what you're trying to do here.  Can you be more specific, please?
(Reporter)

Comment 2

8 years ago
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.
(Reporter)

Comment 3

8 years ago
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.
(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.
(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?
(Reporter)

Comment 6

8 years ago
(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.
(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?
(Reporter)

Comment 8

8 years ago
(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.
(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.
(Reporter)

Comment 10

8 years ago
(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)
(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.  ;-)
(Reporter)

Updated

7 years ago
Summary: HTMLInputElement should change the editor state, not the frame → HTMLInputElement should be changing the editor state instead of nsTextControlFrame
(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!  ;)
You need to log in before you can comment on or make changes to this bug.