Closed Bug 607145 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 7 obsolete files)

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.
blocking2.0: --- → ?
Blocks: 566128
blocking2.0: ? → beta8+
Oh, and its sort of disturbing that we didn't have tests for this.  :(
(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.
This is also going to change how bug 297761 was fixed.
Blocks: 297761
Attached patch WIP Patch (obsolete) — Splinter Review
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).
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #486051 - Attachment is obsolete: true
Attachment #486057 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Given that mozActionURI is removed we might want to push that to b7?
> I still have to found how to have an invalid URI

Invalid in what sense?
(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.
> In sense of the algorithm described here failing:

Try this:  "http://I have spaces/"
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...
(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).
> 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!
And in fact, using GetURIAttr on the submitter element is too, per the same spec item.
Attachment #486057 - Flags: review?(bzbarsky) → review-
(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?
(In reply to comment #15)
> This seems to work and even tested...

content/html/content/test/test_bug392567.html
> 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?
Depends on: 607957
Depends on: 607974
Attached patch Inter diff (obsolete) — Splinter Review
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #486057 - Attachment is obsolete: true
Attachment #486632 - Flags: review?(bzbarsky)
Attached patch Inter diff 2 (obsolete) — Splinter Review
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #486632 - Attachment is obsolete: true
Attachment #486634 - Flags: review?(bzbarsky)
Attachment #486632 - Flags: review?(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
Attachment #486634 - Flags: review?(bzbarsky) → review+
(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.
Attachment #486631 - Attachment is obsolete: true
Attachment #486633 - Attachment is obsolete: true
More unforseen API changes yay!
blocking2.0: beta8+ → beta7+
(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
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.
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.
Attachment #486878 - Flags: review?(bzbarsky)
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #486634 - Attachment is obsolete: true
Attachment #486879 - Flags: review?(bzbarsky)
Attachment #486878 - Attachment description: Patch to fix the tests failures → Inter diff (fixing the tests failures)
Attachment #486878 - Flags: review?(bzbarsky)
Attached patch Inter diff 2Splinter Review
Attached patch Patch v3.1Splinter Review
With comments form IRC.
Attachment #486879 - Attachment is obsolete: true
Attachment #486984 - Flags: review?(bzbarsky)
Attachment #486879 - Flags: review?(bzbarsky)
Comment on attachment 486984 [details] [diff] [review]
Patch v3.1

s/excepted/expected in the comment, and looks good.
Attachment #486984 - Flags: review?(bzbarsky) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Summary: form.action, button.formAction and input.formAction should be reflected as URL → form.action, button.formAction and input.formAction should not be reflected as a simple DOMString
Target Milestone: --- → mozilla2.0b7
Blocks: 608402
You broke mobile again. Is there some way we can be kept in the loop on these changes?
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: