20471 - need to update commands whenever frame state changes in text control

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[buster]

I already have the entry points in place, just need access to the command
dispatcher.  that's bug 20470.

Comment 1

[buster]

without this fix, some menu items will only enable/disable when you do focus
changes, instead of dynamically whenever the user interacts with the text field.
I don't think this is dogfood, so setting for M14.  It is a beta blocker.

Comment 2

[gerardok]

QA Contact update.

Comment 3

[buster]

playing with waterson's fix for 20470, I added this code (condensed here):

  // get the "controllers" object from the text control
  if (controllers)
  {
    nsCOMPtr<nsIDOMXULCommandDispatcher> commandDispatcher;
    result =
       controllers->GetCommandDispatcher(getter_AddRefs(commandDispatcher));
    if (NS_FAILED(result)) { return result; }
    if (commandDispatcher)
    {
      nsAutoString commandString("furby");
      result = commandDispatcher->UpdateCommands(commandString);
    }
  }

but the controllers from the text controls have no command dispatcher associated
with them.  Who is responsible for doing this initial hookup:  calling
SetCommandDispatcher on the controllers of text controls in both chrome and html
content?

Comment 4

[Chris Waterson]

Do HTML edit fields implement nsIControllers? If so, I can eagerly do a setup
on any HTML element that implements the interface. That's probably the piece
that's missing.

Comment 5

[buster]

sorta.

the content node for <textarea> and <input type=text> implement
nsIDOMNSHTMLTextAreaElement and nsIDOMNSHTMLInputElement respectively.  These
interfaces each have a GetControllers() method.  There's a comment in
nsXULCommandDispatcher.cpp that suggests this should be more generic...

nsXULCommandDispatcher::GetControllers(nsIControllers** aResult)
{
  //XXX: we should fix this so there's a generic interface that
  //     describes controllers,
  //     so this code would have no special knowledge of what object
  //     might have controllers.

So we can QI each content node for these interfaces to get controllers if
available, and set the dispatcher on any that we find.
But it sucks to have to QI for more than one interface.

Comment 6

[Chris Waterson]

nsIControllersOwner?

Comment 7

[David Hyatt]

This would be the correct way to do it... make the content nodes implement a
secret interface that would give me another way of getting to the controllers
from the command dispatcher.

You still want the GetControllers methods on the content node interfaces
themselves however for scriptability purposes.

Comment 8

[Chris Waterson]

How about if nsIControllersOwner just had a "SetControllers()" method, so I
could do the hookup?

Comment 9

[Chris Waterson]

Oh duh. I am an idiot. Ignore what I've been saying about "pushing" the
controllers into the HTML element.

buster: what you need to do is set up the command dispatcher in
nsHTMLInputElement's GetControllers() method. You'll need to:

1. QI() the element's document for nsIDOMXULDocument,
2. call GetCommandDispatcher() on that
3. call SetCommandDispatcher() on your new controllers object.

See http://lxr.mozilla.org/mozilla/source/rdf/content/src/nsXULElement.cpp#3321
for code that does this. Copy n' paste, baby.

(Yeah, makes layout depend on XUL, so you might want to #ifdef INCLUDE_XUL
this. Or, like hyatt said, make the super-secret interface, but that seems
like overkill)

Comment 10

[David Hyatt]

Which hookup are we talking about here?

Comment 11

[Chris Waterson]

Setting the command dispatcher in the controllers object when the controllers
object is first created.

Comment 12

[David Hyatt]

Houston, we have a problem.

His text field doesn't occur in a XUL doc necessarily.  It could be nested 26
levels deep inside an HTML doc buried in framesets.

The nsIControllers implementation should use the nsPIDOMWindow interface to walk
up to the topmost window.  It should get the command dispatcher off the XUL doc
in the outermost window and then set it on itself.

All "nsIControllers"s should just do this by default.

Another bug is that all XUL docs get a command dispatcher when only the topmost
XUL doc should have one.  That's one reason i was thinking the command
dispatcher should maybe be on the window instead.

Comment 13

[buster]

This sounds like a bug that waterson or hyatt should own, at least through the
part where the controllers are hooked up to the command dispatcher.  Once that's
done, the editor code is already in place to take advantage of it.  See
nsGfxTextControlFrame::InternalContentChanged()
Guessing M13.

Comment 14

[Chris Waterson]

hyatt: it's all you.

Comment 15

[buster]

adding radha to the cc-list, since her work in the URL bar of the browser is
dependent on this.

Comment 16

[Simon Fraser [no longer active]]

cc: self, since command updating in editor is also impacted.

Comment 17

[David Hyatt]

I can't work on this until saari finishes moving the command dispatcher.  Since
that hasn't happened yet, I'm going to have to push this off to M14.

Comment 18

[David Hyatt]

*** Bug 21376 has been marked as a duplicate of this bug. ***

Comment 19

[Peter Trudelle]

putting on beta1 radar

Comment 20

[ckritzer (gone)]

Updating severity to critical.

Comment 21

[ckritzer (gone)]

Adding beta1 keyword

Comment 22

[leger]

Putting on PDT+ radar for beta1.

Comment 23

[David Hyatt]

Fixed.

Comment 24

[David Hyatt]

Fixed.

Comment 25

[ckritzer (gone)]

Hey Dave,

How do we test this?  Is there a specific testcase where this would manifest
itself?

Thanks dude,

-Kritzer

Comment 26

[buster]

test case is easy.
open mozilla. click in url bar.
open edit menu.  notice what is enabled and disabled.  for example, undo should 
be disabled since you haven't changed anything in the url bar.
click back in the url bar. delete a character
open the edit menu again.  undo should be enabled now, because the text control 
has changed state.

Comment 27

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-02-17-08 Commercial build
- MacOS9 2000-02-16-16 Mozilla build
- Win98 2000-02-16-16 Commercial build

Comment 28

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-02-17-08 Commercial build
- MacOS9 2000-02-16-16 Mozilla build
- Win98 2000-02-16-16 Commercial build