XForms select1 breaks with empty xf:value nodes

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
9 years ago
2 years ago

People

(Reporter: imphil, Assigned: imphil)

Tracking

1.9.1 Branch
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 416339 [details]
XHTML testcase

Consider the following code snipped (see the testcases for complete examples):

  <xf:select1 ref="/Testcase/myvalue">
    <xf:item>
      <xf:label>default label</xf:label>
      <xf:value></xf:value>
    </xf:item>
  </xf:select1>

If the default instance has myvalue set to an empty string, the form should load with "default label" selected in the select1. Right now nothing is selected in XHTML, but the default value is selected in XUL. 

After selecting another select1 item and selecting the first one again (with the empty xf:value), the select1 should display "default label". Right now, XUL and XHTML bindings display no label at all.
(Assignee)

Comment 1

9 years ago
Created attachment 416340 [details]
XUL testcase
(Assignee)

Comment 2

9 years ago
Created attachment 416341 [details] [diff] [review]
patch
Assignee: nobody → mail
Status: NEW → ASSIGNED
Attachment #416341 - Flags: review?(surkov.alexander)
Sorry for the delay in review.

I'm not sure we shouldn't distinguish no nodes case and whitespace nodes cases. For example, I think your patch will select value if

<myvalue></myvalue>

or

<myvalue> </myvalue>

or

<myvalue>
</myvalue>

however it should trigger in the first case only. It looks like these are three different cases and these values should be selected iif item has exactly same value. Does the spec address this issue?
(Assignee)

Comment 4

9 years ago
I tried to preserve the current behavior, which is to ignore whitespace before/after a xf:value content. E.g. right now, this is equivalent:

<myvalue>
test
</myvalue>

<myvalue>test</myvalue>

I didn't find anything in the specs that requires this behavior, no idea why it was initially implemented this way (the relevant code was added in bug 279063).
(In reply to comment #4)
> I tried to preserve the current behavior, which is to ignore whitespace
> before/after a xf:value content. E.g. right now, this is equivalent:
> 
> <myvalue>
> test
> </myvalue>
> 
> <myvalue>test</myvalue>

I think this is ok if there are three nodes in the first example. But I think the following examples should be different

<myvalue> test</myvalue> and <myvalue>test </myvalue> since they contain one node.
(Assignee)

Comment 6

9 years ago
Created attachment 419429 [details] [diff] [review]
patch v2

I've changed the patch a bit so that text content is always taken as-is. No whitespace is stripped before or after the value.
Attachment #416341 - Attachment is obsolete: true
Attachment #416341 - Flags: review?(surkov.alexander)
(Assignee)

Comment 7

9 years ago
Created attachment 419430 [details]
XHTML testcase v2

extended XHTML testcase
Attachment #416339 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #419429 - Flags: review?(surkov.alexander)
That should work however I'm not sure I like new if statement. Also you don't handle theoretical case when few whitespace nodes are presented only. I think we should be out of range in this case. Could you think of the change "if (nonWhitespace) {" clause?

Probably while you're here it's worth to make variable names more consistent like newValue -> stringValue or similar.
(Assignee)

Comment 9

9 years ago
Before I start again, let's summarize what we want to have in the end:

* <xf:value></xf:value> -> newValue = ''
* <xf:value> </xf:value> -> newValue = ' '
* <xf:value>  </xf:value> gives an xforms-out-of-range exception (the same is true for all xf:value nodes with only {2,} whitespace characters)

* <xf:value>test</xf:value> -> newValue = 'test'
* <xf:value> test </xf:value> newValue = ' test '
* <xf:value>test </xf:value> -> newValue = 'test '
* <xf:value>test test</xf:value> gives an xforms-out-of-range exception

is this how it should look?
I'm not sure because your example doesn't seem complete. Technically rule should be 1) ignore whitespace nodes until non whitespace nodes are presented 2) fire out of range if more than one unignored node is presented.
(Assignee)

Comment 11

9 years ago
(In reply to comment #8)
> That should work however I'm not sure I like new if statement.

I tried without it but that would add only more flag variables which doesn't make the code more readable IMO.

> Also you don't
> handle theoretical case when few whitespace nodes are presented only. I think
> we should be out of range in this case. Could you think of the change "if
> (nonWhitespace) {" clause?

How can there be more than one whitespace node and nothing else? Wouldn't that be only one text node then?

> Probably while you're here it's worth to make variable names more consistent
> like newValue -> stringValue or similar.

ok, no problem. I'll have a new patch ready after the other questions are resolved.
(Assignee)

Comment 12

9 years ago
btw, the XUL testcase won't work until bug 539533 is resolved.
(Assignee)

Comment 13

9 years ago
The testcase works again. Alex, could you take a look at the patch?
Comment on attachment 419429 [details] [diff] [review]
patch v2


>+            if (boundNode && boundNode.childNodes.length == 1 &&
>+                boundNode.firstChild.nodeType == Node.TEXT_NODE) {
>+              // If there is only a single text node, we take it as it is.
>+              newValue = boundNode.firstChild.nodeValue;
>+            } else if (boundNode) {
>+              // We are bound to an node without children.
>+              newValue = '';
>+            } else if (boundNode && boundNode.hasChildNodes()) {

it sounds you won't ever get here

could you provide please new algorithm description to prevent the whole bug discussion reading ;) It's hard to remember where we've stopped.
(In reply to comment #11)
> (In reply to comment #8)

> How can there be more than one whitespace node and nothing else? Wouldn't that
> be only one text node then?

It could be done via JS or when you deal with long text, very long textnodes are splited into several ones. It's sort of theoretical case but it would nice if you care while you're here.
Attachment #419429 - Flags: review?(surkov.alexander)
Comment on attachment 419429 [details] [diff] [review]
patch v2

canceling review until comments are addressed.
(Assignee)

Comment 17

8 years ago
Created attachment 495947 [details]
XHTML testcase v3
Attachment #419430 - Attachment is obsolete: true
(Assignee)

Comment 18

8 years ago
Created attachment 495948 [details]
screenshot of Chiba output
(Assignee)

Comment 19

8 years ago
Created attachment 495949 [details]
screenshot of Orbeon output
(Assignee)

Comment 20

8 years ago
Coming back to this bug, I did some testing using Chiba and Orbeon, how they handle whitespace when matching xf:value with the bound value in a select1. Unfortunately, the two implementations handle it differently (see attached screenshots, taken from the updated testcase):

- Orbeon seems to remove all whitespace before and after non-whitespace characters from the contents of xf:value and the bound data. If only whitespace is present, it's completely removed to an empty string.

- Chiba on the other hand takes the string as it is and matches it literally, including whitespace (and newlines).


I think Orbeon's behavior is more intuitive for web developers, as whitespace is usually ignored in HTML, but what's the right solution to this? I'll open a thread at the W3C mailing list, let's see what they think.

Comment 22

8 years ago
http://lists.w3.org/Archives/Public/public-forms/2011Jan/att-0003/2011-01-05.html#topic6

Resolution 2011-01-5.1: We accept the erratum that empty string as matching an empty item in select1 in XForms 1.1.

ACTION-1760 John Boyer to produce the erratum that empty string as matching an empty item in select1 in XForms 1.1.
(Assignee)

Comment 23

8 years ago
Thanks Leigh (and everybody else at the mailing list for their input). I was rather busy the last couple weeks so I couldn't follow up with this one. I'll get a patch ready to make Firefox behave like this "soonish".
RIP xforms
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.