Last Comment Bug 607145 - form.action, button.formAction and input.formAction should not be reflected as a simple DOMString
: form.action, button.formAction and input.formAction should not be reflected a...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b7
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 607957 607974
Blocks: 608402 297761 566128
  Show dependency treegraph
 
Reported: 2010-10-25 16:22 PDT by Mounir Lamouri (:mounir)
Modified: 2010-10-31 20:52 PDT (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
WIP Patch (16.55 KB, patch)
2010-10-26 06:56 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (17.29 KB, patch)
2010-10-26 07:54 PDT, Mounir Lamouri (:mounir)
bzbarsky: review-
Details | Diff | Splinter Review
Inter diff (1.46 KB, patch)
2010-10-28 08:54 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (17.70 KB, patch)
2010-10-28 08:54 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff 2 (1.21 KB, patch)
2010-10-28 08:56 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2.1 (17.53 KB, patch)
2010-10-28 08:57 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review
Inter diff (fixing the tests failures) (12.50 KB, patch)
2010-10-29 03:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v3 (18.81 KB, patch)
2010-10-29 03:13 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Inter diff 2 (10.98 KB, patch)
2010-10-29 12:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v3.1 (20.90 KB, patch)
2010-10-29 12:11 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-10-25 16:22:23 PDT
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 [:bz] (still a bit busy) 2010-10-25 17:11:30 PDT
Oh, and its sort of disturbing that we didn't have tests for this.  :(
Comment 2 Mounir Lamouri (:mounir) 2010-10-25 17:18:51 PDT
(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) 2010-10-26 06:54:46 PDT
This is also going to change how bug 297761 was fixed.
Comment 4 Mounir Lamouri (:mounir) 2010-10-26 06:56:58 PDT
Created attachment 486051 [details] [diff] [review]
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) 2010-10-26 07:54:02 PDT
Created attachment 486057 [details] [diff] [review]
Patch v1
Comment 6 Mounir Lamouri (:mounir) 2010-10-26 07:55:17 PDT
Given that mozActionURI is removed we might want to push that to b7?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2010-10-26 08:33:07 PDT
> I still have to found how to have an invalid URI

Invalid in what sense?
Comment 8 Mounir Lamouri (:mounir) 2010-10-26 08:34:58 PDT
(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 [:bz] (still a bit busy) 2010-10-27 14:34:00 PDT
> In sense of the algorithm described here failing:

Try this:  "http://I have spaces/"
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2010-10-27 14:37:36 PDT
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) 2010-10-28 03:58:38 PDT
(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 [:bz] (still a bit busy) 2010-10-28 06:39:51 PDT
> 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 [:bz] (still a bit busy) 2010-10-28 06:41:12 PDT
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 [:bz] (still a bit busy) 2010-10-28 06:41:59 PDT
And in fact, using GetURIAttr on the submitter element is too, per the same spec item.
Comment 15 Mounir Lamouri (:mounir) 2010-10-28 06:50:28 PDT
(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) 2010-10-28 06:52:31 PDT
(In reply to comment #15)
> This seems to work and even tested...

content/html/content/test/test_bug392567.html
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2010-10-28 07:03:16 PDT
> 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) 2010-10-28 08:54:08 PDT
Created attachment 486631 [details] [diff] [review]
Inter diff
Comment 19 Mounir Lamouri (:mounir) 2010-10-28 08:54:42 PDT
Created attachment 486632 [details] [diff] [review]
Patch v2
Comment 20 Mounir Lamouri (:mounir) 2010-10-28 08:56:38 PDT
Created attachment 486633 [details] [diff] [review]
Inter diff 2
Comment 21 Mounir Lamouri (:mounir) 2010-10-28 08:57:19 PDT
Created attachment 486634 [details] [diff] [review]
Patch v2.1
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2010-10-28 11:13:41 PDT
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) 2010-10-28 11:23:10 PDT
(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] 2010-10-28 12:49:39 PDT
More unforseen API changes yay!
Comment 25 Mounir Lamouri (:mounir) 2010-10-28 14:24:33 PDT
(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) 2010-10-28 16:36:18 PDT
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) 2010-10-29 03:10:42 PDT
Created attachment 486878 [details] [diff] [review]
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) 2010-10-29 03:13:11 PDT
Created attachment 486879 [details] [diff] [review]
Patch v3
Comment 29 Mounir Lamouri (:mounir) 2010-10-29 12:10:19 PDT
Created attachment 486983 [details] [diff] [review]
Inter diff 2
Comment 30 Mounir Lamouri (:mounir) 2010-10-29 12:11:02 PDT
Created attachment 486984 [details] [diff] [review]
Patch v3.1

With comments form IRC.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2010-10-29 12:38:00 PDT
Comment on attachment 486984 [details] [diff] [review]
Patch v3.1

s/excepted/expected in the comment, and looks good.
Comment 32 Mounir Lamouri (:mounir) 2010-10-29 14:39:35 PDT
Push:
http://hg.mozilla.org/mozilla-central/rev/b5faa770249f
Comment 33 Mark Finkle (:mfinkle) (use needinfo?) 2010-10-31 20:51:57 PDT
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?) 2010-10-31 20:52:49 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.