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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla2.0b7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
Blocks: 566128
blocking2.0: ? → beta8+
Oh, and its sort of disturbing that we didn't have tests for this.  :(
(Assignee)

Comment 2

7 years ago
(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.
(Assignee)

Comment 3

7 years ago
This is also going to change how bug 297761 was fixed.
Blocks: 297761
(Assignee)

Comment 4

7 years ago
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).
(Assignee)

Comment 5

7 years ago
Created attachment 486057 [details] [diff] [review]
Patch v1
Attachment #486051 - Attachment is obsolete: true
Attachment #486057 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

7 years ago
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?
(Assignee)

Comment 8

7 years ago
(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...
(Assignee)

Comment 11

7 years ago
(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!
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.
And in fact, using GetURIAttr on the submitter element is too, per the same spec item.
Attachment #486057 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 15

7 years ago
(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?
(Assignee)

Comment 16

7 years ago
(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?
(Assignee)

Updated

7 years ago
Depends on: 607957
(Assignee)

Updated

7 years ago
Depends on: 607974
(Assignee)

Comment 18

7 years ago
Created attachment 486631 [details] [diff] [review]
Inter diff
(Assignee)

Comment 19

7 years ago
Created attachment 486632 [details] [diff] [review]
Patch v2
Attachment #486057 - Attachment is obsolete: true
Attachment #486632 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

7 years ago
Created attachment 486633 [details] [diff] [review]
Inter diff 2
(Assignee)

Comment 21

7 years ago
Created attachment 486634 [details] [diff] [review]
Patch v2.1
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+
(Assignee)

Comment 23

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #486631 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #486633 - Attachment is obsolete: true
More unforseen API changes yay!
blocking2.0: beta8+ → beta7+
(Assignee)

Comment 25

7 years ago
(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
(Assignee)

Comment 26

7 years ago
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.
(Assignee)

Comment 27

7 years ago
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.
Attachment #486878 - Flags: review?(bzbarsky)
(Assignee)

Comment 28

7 years ago
Created attachment 486879 [details] [diff] [review]
Patch v3
Attachment #486634 - Attachment is obsolete: true
Attachment #486879 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
Attachment #486878 - Attachment description: Patch to fix the tests failures → Inter diff (fixing the tests failures)
Attachment #486878 - Flags: review?(bzbarsky)
(Assignee)

Comment 29

7 years ago
Created attachment 486983 [details] [diff] [review]
Inter diff 2
(Assignee)

Comment 30

7 years ago
Created attachment 486984 [details] [diff] [review]
Patch v3.1

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+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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
(Assignee)

Comment 32

7 years ago
Push:
http://hg.mozilla.org/mozilla-central/rev/b5faa770249f
(Assignee)

Updated

7 years ago
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.