Closed Bug 528686 Opened 15 years ago Closed 14 years ago

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

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a5

People

(Reporter: alexey.gaidukov, Assigned: ehsan.akhgari)

References

Details

Attachments

(3 files, 1 obsolete file)

testcase
1.54 KB, application/xhtml+xml
Details
screenshot of DOM Inspector
153.94 KB, image/png
Details
Patch (v1)
4.02 KB, patch
dbaron
: review+
christian
: approval1.9.2.7-
Details | Diff | Splinter Review 
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
Attached file testcase — 

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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).

    
    

    
  
Olli, you know better event dispatcher code than me, could you look at the problem, please?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: xf:select doesn't work if in html there is contenteditable element → xf:select doesn't work if contenteditable element is presented within the document

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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;
> }

    
    

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

    
    

    
  
Aaron, do you have any ideas about this?

    
    

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

    
    

    
  
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.

    
    

    
  
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.

    
    

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

    
    

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

    
    

    
  
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)?

    
    

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

    
    

    
  
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.

    
    

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

Are you sure that's actually the problem?

    
    

    
  
(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?

    
    

    
  
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.

    
    

    
  


  
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.

    
    

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

    
    

    
  
> 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)

    
    

    
  
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?

    
    

    
  
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.

    
    

    
  
(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?

    
    

    
  
you have the xforms plugin loaded?

    
    

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

Yes, it's loaded.

    
    

    
  
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!

    
    

    
  
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)

    
    

    
  
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.

    
    

    
  
I can build mozilla-central with XForms any time.

    
    

    
  
(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.

    
    

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

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Possible fix? (obsolete)
        — 
          — Splinter Review
      

    


  
Sergey, could you please try this patch?

    
    

    
  
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.

    
    

    
  
(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.

    
    

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

    
    

    
  
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 {

    
    

    
  
(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?

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (v1)
        — 
          — Splinter Review
      

    


  
Fair enough.
Assignee: nobody → ehsan

        Attachment #443764 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #444001 -
        Flags: review?(dbaron)

    
    

    
  
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).

    
    

    
  
(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 on attachment 444001 [details] [diff] [review]
Patch (v1)

r=dbaron

        Attachment #444001 -
        Flags: review?(dbaron) → review+

    
    

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

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/36b3e529f9db
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

    
    

    
  
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 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!

        Attachment #444001 -
        Flags: approval1.9.2.5?

    
    

    
  
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.

        Attachment #444001 -
        Flags: approval1.9.2.7?

        Attachment #444001 -
        Flags: approval1.9.2.6-

        Attachment #444001 -
        Flags: approval1.9.2.5?

        Attachment #444001 -
        Flags: approval1.9.2.9?
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: