Closed Bug 1064063 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Eduard.Braun2, Assigned: longsonr)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → longsonr
Attached patch switch.txt (obsolete) — Splinter Review
Attachment #8485628 - Flags: review?(dholbert)
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.
Depends on: 484176
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.
Attachment #8485628 - Flags: review?(dholbert) → review-
(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)
Attached patch switch.txtSplinter Review
Should read my own documentation ;-)
Attachment #8485628 - Attachment is obsolete: true
Attachment #8486037 - Flags: review?(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 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.
Attachment #8486037 - Flags: review?(dholbert) → review+
(and with comment 6 addressed)
https://hg.mozilla.org/mozilla-central/rev/7ddc6addda4c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.