528686 - xf:select doesn't work if contenteditable element is presented within the document

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Alexey Gaidukov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)

xf:select doesn't work if in all appearance if in html there is contenteditable element. xf:select1 works ok in all appearance.

Reproducible: Always

Steps to Reproduce:
1. Open attached testcase
2. Try to change contol's value by mouse
3.
Actual Results:  
Control doesn't change it's value

Comment 1

[Alexey Gaidukov]

Attachment: testcase

Comment 2

[Sergey Reymerov]

I looked at selects-xhtml.xml#select-full-item.
It contain event click handler. But that method handle clicks only for <children/> except <html:input type="checkbox" anonid="control"/>. I think that bug is duplicate of bug 490367.

Comment 3

[Sergey Reymerov]

I did debug FF. I took two calltrees of normal work and ugly work. That calltrees are different in <that place>. 
  gklayout.dll!nsXBLEventHandler::HandleEvent() 
  gklayout.dll!nsEventListenerManager::HandleEventSubType() 
  gklayout.dll!nsEventListenerManager::HandleEvent() 
  gklayout.dll!nsEventTargetChainItem::HandleEvent() 
  gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain() 
  <that place>
  gklayout.dll!nsEventDispatcher::Dispatch() 
  gklayout.dll!PresShell::HandleEventInternal() 
  gklayout.dll!PresShell::HandlePositionedEvent() 
  gklayout.dll!PresShell::HandleEvent() 
  ...

I look deeply in code (nsEventDispatcher.cpp, line 531) :  
nsEventChainPreVisitor preVisitor(aPresContext, aEvent, aDOMEvent, status,          isInAnon);  
targetEtci->PreHandleEvent(preVisitor);
if (preVisitor.mCanHandle) {
  \\ Normal work in that block
}

After this I tried reduce XForms code for that bug (leave only bindings selects-xhtml.xml#select-full and selects-xhtml.xml#select-full-item and css-styles for this). Bug remains.

It's neither XBL error nor XForms binding error. It could be error of implementation of nsXFormsSelectElement.

Comment 4

[Sergey Reymerov]

I've found differences between two versions of page (with contenteditable and without). 
It is in nsEventDispatcher::Dispatch (nsEventDispatcher.cpp:531).

>  nsEventChainPreVisitor preVisitor(aPresContext, aEvent, aDOMEvent, status,
>                                    isInAnon);
>  targetEtci->PreHandleEvent(preVisitor);
>
>  if (preVisitor.mCanHandle) {

In non contenteditable mCanHandle is true and if not it is false. 

I looked at targetEtci->PreHandleEvent and in nsHTMLInputElement::PreHandleEvent (nsHTMLInputElement.cpp:1579) found that code:

>  if (disabled) {
>    return NS_OK;
>  }
>  
>  // For some reason or another we also need to check if the style shows us
>  // as disabled.
>  {
>    nsIFrame* frame = GetPrimaryFrame();
>    if (frame) {
>      const nsStyleUserInterface* uiStyle = frame->GetStyleUserInterface();
>
>      if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE ||
>          uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED) {
>        return NS_OK;
>      }
>    }
>  }

In contenteditable version disabled is false. And in non contenteditable version disabled is true. 
And code below defines disabled variable (and I don't understand how it works but it shows debuger).

Comment 5

[alexander :surkov (:asurkov)]

Olli, you know better event dispatcher code than me, could you look at the problem, please?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #0)

> xf:select doesn't work if in all appearance if in html there is contenteditable
> element. xf:select1 works ok in all appearance.

checkboxes don't work, however if I click on the label then checkbox value is changed.

Comment 7

[Sergey Reymerov]

I've just returned to that bug. I knew that bug appears somewhere in that chain:
> nsHTMLInputElement::PreHandleEvent 
> nsHTMLInputElement::GetDisabled()
> nsGenericHTMLElement::GetBoolAttr
> nsGenericElement::HasAttr
> nsAttrAndChildArray::IndexOfAttr
When bug appears GetDisabled() returns true because HasAttr returns true (for "disabled" attribute). 
When bug doesn't appear GetDisabled() returns false.

Comment 8

[Sergey Reymerov]

Bug appears because in nsHTMLInputElement::PreHandleEvent perform next return:
> if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE ||
>     uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED) {
>   return NS_OK;
> }

Comment 9

[Sergey Reymerov]

Olli, could you give an opinion about my last messages?

Comment 10

[Sergey Reymerov]

Aaron, do you have any ideas about this?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Sergey, do you know ehy mUserInput is none or disabled?
I think it should have those values only inside contentEditable.

Comment 12

[Sergey Reymerov]

Frame of checkbox has attribute uiStyle->mUserInput that equal  NS_STYLE_USER_INPUT_NONE. So nsHTMLInputElement:reHandleEvent doesn't handle any events on that checkbox'es.

Comment 13

[Sergey Reymerov]

I've just found some interesting thing. I've saw (via DOM Inspector) that xf:select has next CSS rule (from resource://gre/res/contenteditable.css):
> select:-moz-read-write,
> :-moz-read-write > input[disabled],
> :-moz-read-write > input[type="checkbox"],
> :-moz-read-write > input[type="radio"],
> :-moz-read-write > input[type="file"],
> input[contenteditable="true"][disabled],
> input[contenteditable="true"][type="checkbox"],
> input[contenteditable="true"][type="radio"],
> input[contenteditable="true"][type="file"] {
>   -moz-user-select: all !important;
>   -moz-user-input: none !important;
>   -moz-user-focus: none !important;
> }

So, bug appears that this style applies to xf:select when contentEditable is true.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

But the contentEditable div is outside the xf:select, so why does that apply to
xf:select?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

To fix this, would it be enough to hack xforms.css a bit?

Comment 16

[Sergey Reymerov]

I think xforms.css isn't enough to fix (I think xforms.css isn't guilty). It looks like Gecko thinks xf:select in <div contenteditable="true">. Where Gecko applies styles to elements (when contenteditable state changes)?

Comment 17

[alexander :surkov (:asurkov)]

Ehsan, could you please look at the problem (comment #13, comment #14)? It doesn't sound like xforms problem.

Comment 18

[(no longer active)]

I tried adding:

@namespace url("http://www.w3.org/1999/xhtml");

to the beginning of contenteditable.css to make sure that the rule in comment 13 does not match xf:select, but it didn't work.

CCing dbaron to see if he knows how to make that rule only match HTML select elements.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That should work?  Did you rebuild whatever jar files are necessary after making that change?

Are you sure that's actually the problem?

Comment 20

[(no longer active)]

(In reply to comment #19)
> That should work?  Did you rebuild whatever jar files are necessary after
> making that change?

Yes.  I verified that the CSS when accessed from the gre-resources URI contains that namespace declaration.

> Are you sure that's actually the problem?

Sergey said in comment 13 that this rule is applied to xf:select.  I have not been able to see any CSS rules inside DOM Inspector myself, but if this rule is applied to the xf:select element, then there's a good chance that it's the source of this problem.

So, when we load an override stylesheet without a @namespace declaration, what types of elements do its rules match?  Is it possible that they would match anything from the xforms namespace?

Sergey, how did you view the applied rules in DOM Inspector?

Comment 21

[(no longer active)]

Also, if you remove the contenteditable attribute using DOM Inspector, does the problem get fixed?  Frankly I have no idea what a proper xf:select should look like.

Comment 22

[Sergey Reymerov]

Attachment: screenshot of DOM Inspector

This is double-screenshot of my DOM Inspector that shows applied styles for xf:select.
When I change contentEditable state via DOM Inspector this styles apply and delete when I add this attribute and remove, respectively.

Comment 23

[(no longer active)]

Hmm, which version of Firefox are you using?  For me on trunk, the applied rules list is empty.

Comment 24

[Sergey Reymerov]

> Hmm, which version of Firefox are you using?  For me on trunk, the applied
> rules list is empty.

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.5pre) Gecko/20100429 Namoroka/3.6.5pre(In reply to comment #23)

Comment 25

[(no longer active)]

Hmm, strange.  I tried it with a 3.6 version of Firefox as well, and I still can't get it to show up in DOM Inspector.

Anyways, based on comment 22 and the attached screenshot, it seems to be clear that the contenteditable.css rules are the culprit here.

dbaron, do you have any idea how we can prevent them from being applied to xf:select?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The @namespace declaration should work.  I couldn't find any evidence that the rules in contenteditable.css were being applied at all, though, when I loaded the testcase.

Comment 27

[Sergey Reymerov]

(In reply to comment #25)
> Hmm, strange.  I tried it with a 3.6 version of Firefox as well, and I still
> can't get it to show up in DOM Inspector.

I use DOM Inspector 2.0.4 with Gecko/20100429 and Linux Ubuntu 10.04. Maybe some of this could be reason that you couldn't see applied styles to xf:select?

Comment 28

[Philipp Wagner [:imphil]]

you have the xforms plugin loaded?

Comment 29

[Sergey Reymerov]

(In reply to comment #28)
> you have the xforms plugin loaded?

Yes, it's loaded.

Comment 30

[(no longer active)]

So I didn't know that I needed the xforms addon.  I installed it from AMO on 3.6 (even though it wasn't marked as compatible), and I crash with that.

Sergey, would you mind giving me a set of accurate steps to reproduce this problem?  Like, starting with a blank profile?  Thanks!

Comment 31

[Alexey Gaidukov]

I can reproduce the bug on FF 3.6.3 with xforms addong from attachement of bug 539184 (https://bugzilla.mozilla.org/attachment.cgi?id=437883)

Comment 32

[(no longer active)]

Does someone who has the xforms addon working want to help us fix this?  You will require a clone of mozilla-central which you can build, and I'd be happy to send you patches.

Comment 33

[Sergey Reymerov]

I can build mozilla-central with XForms any time.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> I can build mozilla-central with XForms any time.

Keep in mind there are compilation errors when you build xforms on mozilla-central.

Comment 35

[Sergey Reymerov]

Ooou, I've forgot that I compile mozilla-1.9.2.

Comment 36

[Sergey Reymerov]

Ehsan I can put your patches (may be manually) to current stable version and then examine it.

Comment 37

[(no longer active)]

Attachment: Possible fix?

Sergey, could you please try this patch?

Comment 38

[(no longer active)]

FWIW, I tried building mozilla-central with --enable-extensions=xforms (I'm not sure if that's enough though.)

Without the XForms addon, I don't see anything different in the test case than a normal mozilla-central build.  With the XForms addon, I crash when I try to view the test case, with this stack:

0   libxforms.dylib               	0x07e4e863 0x7e41000 + 55395
1   libxforms.dylib               	0x07e4e9f1 0x7e41000 + 55793
2   libgklayout.dylib             	0x05b88daf nsXTFElementWrapper::BindToTree(nsIDocument*, nsIContent*, nsIContent*, int) + 227 (nsXTFElementWrapper.cpp:185)
3   libgklayout.dylib             	0x056370a9 nsGenericElement::doInsertChildAt(nsIContent*, unsigned int, int, nsIContent*, nsIDocument*, nsAttrAndChildArray&) + 1523 (nsGenericElement.cpp:3378)
4   libgklayout.dylib             	0x05637349 nsGenericElement::InsertChildAt(nsIContent*, unsigned int, int) + 127 (nsGenericElement.cpp:3324)
5   libgklayout.dylib             	0x052829bf nsINode::AppendChildTo(nsIContent*, int) + 63 (nsINode.h:500)
6   libgklayout.dylib             	0x057ce37d nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int, int) + 1551 (nsXMLContentSink.cpp:1075)
7   libgklayout.dylib             	0x057cef0e nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int) + 60 (nsXMLContentSink.cpp:990)
8   libhtmlpars.dylib             	0x04cd0f00 nsExpatDriver::HandleStartElement(unsigned short const*, unsigned short const**) + 240 (nsExpatDriver.cpp:410)
9   libhtmlpars.dylib             	0x04cd0f86 Driver_HandleStartElement(void*, unsigned short const*, unsigned short const**) + 100 (nsExpatDriver.cpp:98)
10  libhtmlpars.dylib             	0x04d03a5e doContent + 2268 (xmlparse.c:2438)
11  libhtmlpars.dylib             	0x04d02dad contentProcessor + 81 (xmlparse.c:2095)
12  libhtmlpars.dylib             	0x04d07b28 doProlog + 2626 (xmlparse.c:4075)
13  libhtmlpars.dylib             	0x04d070e0 prologProcessor + 143 (xmlparse.c:3811)
14  libhtmlpars.dylib             	0x04d06b58 prologInitProcessor + 88 (xmlparse.c:3626)
15  libhtmlpars.dylib             	0x04d02158 MOZ_XML_Parse + 551 (xmlparse.c:1528)
16  libhtmlpars.dylib             	0x04ccf219 nsExpatDriver::ParseBuffer(unsigned short const*, unsigned int, int, unsigned int*) + 543 (nsExpatDriver.cpp:1003)
17  libhtmlpars.dylib             	0x04cd03a7 nsExpatDriver::ConsumeToken(nsScanner&, int&) + 1045 (nsExpatDriver.cpp:1102)
18  libhtmlpars.dylib             	0x04ce745e nsParser::Tokenize(int) + 332 (nsParser.cpp:3091)
19  libhtmlpars.dylib             	0x04cee6b4 nsParser::ResumeParse(int, int, int) + 510 (nsParser.cpp:2323)
20  libhtmlpars.dylib             	0x04ceb8a2 nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 790 (nsParser.cpp:2955)
21  libdocshell.dylib             	0x06eb1b55 nsDocumentOpenInfo::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 99 (nsURILoader.cpp:306)
22  libnecko.dylib                	0x0480270c nsHttpChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 802 (nsHttpChannel.cpp:5384)
23  libnecko.dylib                	0x04720cfb nsInputStreamPump::OnStateTransfer() + 759 (nsInputStreamPump.cpp:510)
24  libnecko.dylib                	0x0472124e nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 138 (nsInputStreamPump.cpp:400)
25  libxpcom_core.dylib           	0x004b9638 nsInputStreamReadyEvent::Run() + 100 (nsStreamUtils.cpp:113)
26  libxpcom_core.dylib           	0x004eeb6e nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:527)
27  libxpcom_core.dylib           	0x004700f9 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 146 (nsThreadUtils.cpp:200)
28  libwidget_mac.dylib           	0x0516a3b9 nsBaseAppShell::NativeEventCallback() + 181 (nsBaseAppShell.cpp:127)
29  libwidget_mac.dylib           	0x051153d1 nsAppShell::ProcessGeckoEvents(void*) + 497 (nsAppShell.mm:395)
30  com.apple.CoreFoundation      	0x926da15b __CFRunLoopDoSources0 + 1563
31  com.apple.CoreFoundation      	0x926d7c1f __CFRunLoopRun + 1071
32  com.apple.CoreFoundation      	0x926d70f4 CFRunLoopRunSpecific + 452
33  com.apple.CoreFoundation      	0x926d6f21 CFRunLoopRunInMode + 97
34  com.apple.HIToolbox           	0x916c20fc RunCurrentEventLoopInMode + 392
35  com.apple.HIToolbox           	0x916c1ded ReceiveNextEventCommon + 158
36  com.apple.HIToolbox           	0x916c1d36 BlockUntilNextEventMatchingListInMode + 81
37  com.apple.AppKit              	0x928d9135 _DPSNextEvent + 847
38  com.apple.AppKit              	0x928d8976 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
39  com.apple.AppKit              	0x9289abef -[NSApplication run] + 821
40  libwidget_mac.dylib           	0x05114123 nsAppShell::Run() + 291 (nsAppShell.mm:747)
41  libtoolkitcomps.dylib         	0x07704d86 nsAppStartup::Run() + 148 (nsAppStartup.cpp:184)
42  XUL                           	0x00012509 XRE_main + 11908 (nsAppRunner.cpp:3564)
43  org.mozilla.minefielddebug    	0x00002629 main + 714 (nsBrowserApp.cpp:158)
44  org.mozilla.minefielddebug    	0x000022aa start + 54

The crash might be expected, since the XForms addon version on AMO is marked as compatible with Firefox 3.0.

Comment 39

[(no longer active)]

(In reply to comment #37)
> Created an attachment (id=443764) [details]
> Possible fix?
> 
> Sergey, could you please try this patch?

You could apply this patch manually to the contenteditable.css file found in the res directory under the Firefox directory for a build which works.  You don't really need to build mozilla-central for the purpose of testing this patch.

Comment 40

[Sergey Reymerov]

I've tried this patch and it's fix that bug. I think that patch could be commit to hg.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that if we want to land this patch, it might require some additional adjustment.  For example, selectors like these:

:-moz-read-write > input:-moz-read-only,
:-moz-read-write > button:-moz-read-only,
:-moz-read-write > textarea:-moz-read-only {

may need to become:

*|*:-moz-read-write > input:-moz-read-only,
*|*:-moz-read-write > button:-moz-read-only,
*|*:-moz-read-write > textarea:-moz-read-only {

Comment 42

[(no longer active)]

(In reply to comment #41)
> I'd note that if we want to land this patch, it might require some additional
> adjustment.  For example, selectors like these:
> 
> :-moz-read-write > input:-moz-read-only,
> :-moz-read-write > button:-moz-read-only,
> :-moz-read-write > textarea:-moz-read-only {
> 
> may need to become:
> 
> *|*:-moz-read-write > input:-moz-read-only,
> *|*:-moz-read-write > button:-moz-read-only,
> *|*:-moz-read-write > textarea:-moz-read-only {

Considering the fact that we don't support editing elements which are not in the HTML namespace, is this change really necessary?

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure.  It might make more sense conceptually, though...

Comment 44

[(no longer active)]

Attachment: Patch (v1)

Fair enough.

Comment 45

[Sergey Reymerov]

I've tested patch (v1). And it's normally fix bug. 
But now I have a question: what behaviour must be for xf-elements in div-element with contentEditable=true? 
At the moment xf-select in div-element with contentEditable=true behaves like it behaved before patch applied (click on checkbox doesn't change checkbox state but state is changed by label click).

Comment 46

[(no longer active)]

(In reply to comment #45)
> I've tested patch (v1). And it's normally fix bug. 
> But now I have a question: what behaviour must be for xf-elements in
> div-element with contentEditable=true? 
> At the moment xf-select in div-element with contentEditable=true behaves like
> it behaved before patch applied (click on checkbox doesn't change checkbox
> state but state is changed by label click).

I don't think that the behavior of non-HTML elements inside contenteditable elements is actually defined.  But if you can think of how the element is supposed to behave, feel free to file another bug.

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 444001 [details] [diff] [review]
Patch (v1)

r=dbaron

Comment 48

[Sergey Reymerov]

I think this patch could be laned in 1.9.2 because I test it in 1.9.2.

Comment 49

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/36b3e529f9db

Comment 50

[(no longer active)]

Comment on attachment 444001 [details] [diff] [review]
Patch (v1)

(In reply to comment #48)
> I think this patch could be laned in 1.9.2 because I test it in 1.9.2.

I'll request approval for this patch on branch, then.

Comment 51

[(no longer active)]

Comment on attachment 444001 [details] [diff] [review]
Patch (v1)

(In reply to comment #50)
> (From update of attachment 444001 [details] [diff] [review])
> (In reply to comment #48)
> > I think this patch could be laned in 1.9.2 because I test it in 1.9.2.
> 
> I'll request approval for this patch on branch, then.

Heh!  I forgot to do that!

Comment 52

[LegNeato]

Comment on attachment 444001 [details] [diff] [review]
Patch (v1)

We will not be taking this for 3.6.6. Moving approval request forward.

If you disagree, send me an email.