1064063 - SVG <switch> elements ignore elements with empty "systemLanguage" attribute when "allowReorder" attribute is set

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Patrick Storz]

Attachment: Testcase for the bug (instead of defaulting to "other" no text is shown at all)

If the "allowReorder" attribute of an SVG <switch> element is set, Firefox fails to default to a child that has no "systemLanguage" attribute set.

E.g. have a look at the testcase:
If the system language equals "xy" (which should be false on most systems ;) ), the switch should instead default to the "other" element with no "systemLanguage" set.

Instead, Firefox shows no text at all.

Comment 1

[Robert Longson [:longsonr]]

Attachment: switch.txt

Comment 2

[Robert Longson [:longsonr]]

http://www.w3.org/TR/smil/smil-content.html#adef-allowReorder - Any final child without systemLanguage attribute should retain its place as the default item to present.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8485628 [details] [diff] [review]
switch.txt

The spec text "any final child without systemLanguage" sounds a bit weird to me -- does that mean the child has to actually *be* the final child?

I'm guessing it's wanting to say "the last child that has no systemLanguage attribute"... That makes the most sense to me.

Anyway: based on that interpretation, I think the patch might need tweaking. Quoting the patch:

>           int32_t languagePreferenceRank =
>               tests->GetBestLanguagePreferenceRank(acceptLangs);
>           switch (languagePreferenceRank) {
[...]
>           case -1:
>-            // not found
>+            // no system language, we'll use the last child as the default
>+            defaultChild = child;
>             break;

"no system language" is wrong there, I think... -1 doesn't mean the child has no systemLanguage attribute. It just means the child's systemLanguage doesn't match any in acceptLangs, according to http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTests.h#45

I think it does the wrong thing, too -- if my interpretation of allowReorder is correct, we'd really only want to use |child| there if it had *no* systemLanguage attribute (as opposed to an attribute that simply doesn't match).

In particular, I think this patch would make us do the wrong thing in a case like this:
  <switch allowReorder="yes">
    <text systemLanguage="invalid">Do not choose me</text>
  </switch>

...and...
  <switch allowReorder="yes">
    <text systemLanguage="invalid">Do not choose me</text>
    <text>CHOOSE ME</text>
    <text systemLanguage="invalid">Do not choose me</text>
  </switch>

From my reading of the spec, I think we should choose nothing in the first example, and we should choose the second child in the second example.  But I think your patch would choose the final child in both cases.

Comment 4

[Daniel Holbert [:dholbert]]

(Could you include tests for those ^ example scenarios with your updated patch, if we don't already have them?) (unless I'm mistaken about the expected behavior here)

Comment 5

[Robert Longson [:longsonr]]

Attachment: switch.txt

Should read my own documentation ;-)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8486037 [details] [diff] [review]
switch.txt

>+++ b/content/svg/content/test/test_switch.xhtml
>+    /* test that we use the default if nothing matches */
>+    second.setAttribute("systemLanguage", "fr");
>+    checkWidth(s, 80);
>+    
[...]
>+
>+    /* test that we use the last item if no others match */
>+    first.setAttribute("systemLanguage", "fr");
>+    second.setAttribute("systemLanguage", "fr");
>+    third.removeAttribute("systemLanguage");
>+    checkWidth(s, 80);

I think these ^ two checks are redundant (testing the same thing).

In both cases, the first & second items have systemLanguage="fr" and the third has no systemLanguage attribute, and we're testing that we render the third.

So, probably remove the latter one, unless it does something different that I'm not seeing.  (You probably want to preserve the |third.removeAttribute("systemLanguage");| call for the subsequent check, though.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8486037 [details] [diff] [review]
switch.txt

>diff --git a/content/svg/content/src/SVGSwitchElement.cpp b/content/svg/content/src/SVGSwitchElement.cpp
>+          case -2:
>+            // no system language attribute
>+            defaultChild = child;

Two nits:
 1. s/system language/systemLanguage/ (the actual attribute-name), for preciseness/clarity

 2. Add ". We'll use the last such child, if we don't find anything better" (or something along those lines) to the end of the comment.

r=me with that.

Comment 8

[Daniel Holbert [:dholbert]]

(and with comment 6 addressed)

Comment 9

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=340f4bd4e3ca

Comment 10

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ddc6addda4c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ddc6addda4c