607145 - form.action, button.formAction and input.formAction should not be reflected as a simple DOMString

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

I changed how form.action was reflected and this change was wrong :(

This should be a blocker because of the regression in form.action behavior.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and its sort of disturbing that we didn't have tests for this.  :(

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> Oh, and its sort of disturbing that we didn't have tests for this.  :(

We had but they have been changed in bug 566128.

I should be blamed.

Comment 3

[Mounir Lamouri (:mounir)]

This is also going to change how bug 297761 was fixed.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

I still have to found how to have an invalid URI (it should return the empty string in that case). In addition, I'm wondering if when @action isn't set .action should return location.href (like when @action equals the empty string).

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 6

[Mounir Lamouri (:mounir)]

Given that mozActionURI is removed we might want to push that to b7?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I still have to found how to have an invalid URI

Invalid in what sense?

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> > I still have to found how to have an invalid URI
> 
> Invalid in what sense?

In sense of the algorithm described here failing:
http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#resolve-a-url

I can change the naming to "can't be resolved" if you prefer.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> In sense of the algorithm described here failing:

Try this:  "http://I have spaces/"

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486057 [details] [diff] [review]
Patch v1

Doesn't this patch regress the behavior of action=""?

I didn't read the test changes yet...

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> > In sense of the algorithm described here failing:
> 
> Try this:  "http://I have spaces/"

That's what I did: "http://a b"

(In reply to comment #10)
> Comment on attachment 486057 [details] [diff] [review]
> Patch v1
> 
> Doesn't this patch regress the behavior of action=""?

It changes the value returned by .action but the behavior is the same AFAICT.

I'm wondering if we shouldn't try to have this for b7 given that it's changing the interfaces (removing mozActionURI).

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It changes the value returned by .action but the behavior is the same AFAICT.

It changes the behavior of GetActionURL, no?

Before this patch, action="" returned an empty string from GetMozActionUri, and therefore GetActionURL returned the document URI.

After this patch, action="" returns whatever GetAction() returns in that case, which looks to me like it will be the base URI of the node (affected by xml:base and <base> tags).  We even have explicit comments about this issue, which you delete in this patch!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that the current HTML5 spec says that too.  http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#concept-form-submit step 9.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And in fact, using GetURIAttr on the submitter element is too, per the same spec item.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> > It changes the value returned by .action but the behavior is the same AFAICT.
> 
> It changes the behavior of GetActionURL, no?
> 
> Before this patch, action="" returned an empty string from GetMozActionUri, and
> therefore GetActionURL returned the document URI.
> 
> After this patch, action="" returns whatever GetAction() returns in that case,
> which looks to me like it will be the base URI of the node (affected by
> xml:base and <base> tags).  We even have explicit comments about this issue,
> which you delete in this patch!

Yes, I've removed references to bug 297761 because it sounds to me that bug 297761 doesn't exist with this patch. AFAIUI, we can test bug 297761 with a simple <form action=""> and check that it's submitted to the current document and not the base URL, right?
This seems to work and even tested... So, what am I misunderstanding?

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> This seems to work and even tested...

content/html/content/test/test_bug392567.html

Comment 17

[Boris Zbarsky [:bzbarsky]]

> AFAIUI, we can test bug 297761 with a
> simple <form action=""> and check that it's submitted to the current document
> and not the base URL, right?

Yes.

> content/html/content/test/test_bug392567.html

The base URI and the document URI are the same in this testcase, no?

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Inter diff 2

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486634 [details] [diff] [review]
Patch v2.1

>+  // looks like no attribute have a specified default value.

s/have/has/

>+++ b/content/html/content/test/test_bug392567.html

The changes here look suspect (and in particular, need to be merged with the other stuff I already reviewed.

>-  is(form.action, tests[currentTest][0],
>-     "action IDL attribute should reflect the action content attribute");

Why is this going away?

Is it really desirable for action and formaction IDL props to not have the real action URI in the "" case?  Spec issue?

Add a test for formaction="" when base is changed?

r=me modulo the test issues above

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> >+++ b/content/html/content/test/test_bug392567.html
> 
> The changes here look suspect (and in particular, need to be merged with the
> other stuff I already reviewed.
> 
> >-  is(form.action, tests[currentTest][0],
> >-     "action IDL attribute should reflect the action content attribute");
> 
> Why is this going away?

This is now tested here:
content/html/content/test/test_bug607145.html

> Is it really desirable for action and formaction IDL props to not have the real
> action URI in the "" case?  Spec issue?

When action was set to the empty string we were returning the empty string. Now, we would return the resolved URL but not the one where the submission will go to. I'm not sure that's a big deal actually. It would be better to have it really reflecting the submission URL but I can imagine the answer would be "no one really care" and that's probably true. Though, submitting a bug to the spec doesn't cost so much so I will do that ;)

> Add a test for formaction="" when base is changed?

Ok.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

More unforseen API changes yay!

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> When action was set to the empty string we were returning the empty string.
> Now, we would return the resolved URL but not the one where the submission will
> go to. I'm not sure that's a big deal actually. It would be better to have it
> really reflecting the submission URL but I can imagine the answer would be "no
> one really care" and that's probably true. Though, submitting a bug to the spec
> doesn't cost so much so I will do that ;)

http://www.w3.org/Bugs/Public/show_bug.cgi?id=11161

Comment 26

[Mounir Lamouri (:mounir)]

This patch is failing two tests. A DOM level 2 test that can be fixed easily.
The other one is in the password manager because now if @action contains an invalid URL, .action returns "" instead of @action value so the password manager code changes the url to the document location.

I've also realized there is another issue with this patch: if you try to submit <form action="http://a b/"> it will submit to the current document.

None of these issues are big deals and they can be easily fixed but given that we want to have the interface change to b7, I will update that patch to have .action behaving like it is in Gecko 1.9.2 and I will open follow-ups to have it working like HTML5 specs want.

Comment 27

[Mounir Lamouri (:mounir)]

Attachment: Inter diff (fixing the tests failures)

So, basically, this is making .action behaving exactly like .mozActionURI and .action in Gecko 1.9.2. That way, it will not break anything for b7 and we will have time to think about the edge cases of the reflection for b8 (when @action is the empty string and when the URL is invalid).

I've updated dom/tests/mochitest/dom-level2-html/test_button03.html to check our current behavior. In 1.9.2, it was a todo for something that is clearly not what HTML5 specify.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

Comment 29

[Mounir Lamouri (:mounir)]

Attachment: Inter diff 2

Comment 30

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

With comments form IRC.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486984 [details] [diff] [review]
Patch v3.1

s/excepted/expected in the comment, and looks good.

Comment 32

[Mounir Lamouri (:mounir)]

Push:
http://hg.mozilla.org/mozilla-central/rev/b5faa770249f

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

You broke mobile again. Is there some way we can be kept in the loop on these changes?

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #33)
> You broke mobile again. Is there some way we can be kept in the loop on these
> changes?

I suppose we should have mobile specific tests for this and not blame anyone but ourselves.