HTMLFormElement action IDL attribute should reflect the action content attribute

RESOLVED FIXED in mozilla2.0b5

Status

()

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

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b5
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

8 years ago
|GetURIAttr| should be done outsite of |GetAction|, in other words, it should be done by callers if needed.
(Assignee)

Updated

8 years ago
Summary: HTMLFormElement accept IDL attribute should reflect the target content attribute → HTMLFormElement accept IDL attribute should reflect the accept content attribute
(Assignee)

Updated

8 years ago
Blocks: 566160
(Assignee)

Updated

8 years ago
Summary: HTMLFormElement accept IDL attribute should reflect the accept content attribute → HTMLFormElement action IDL attribute should reflect the action content attribute
(Assignee)

Comment 1

8 years ago
Created attachment 445664 [details] [diff] [review]
Tests v1
Assignee: nobody → mounir.lamouri
Attachment #445664 - Flags: review?(jonas)
(Assignee)

Comment 2

8 years ago
Created attachment 445665 [details] [diff] [review]
Patch v1
Attachment #445665 - Flags: review?(jonas)
Attachment #445665 - Flags: review?(jonas) → review+
Attachment #445664 - Flags: review?(jonas) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
I landed this, but backed it out on suspicion of causing test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275354026.1275354385.28784.gz

It also caused an UNEXPECTED PASS in test_button03.html.
Keywords: checkin-needed
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> I landed this, but backed it out on suspicion of causing test failures:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1275354026.1275354385.28784.gz

Actually, the password manager relies on the current wrong behavior, see:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#971
I suppose the password manager should create the URI with the form page URL + the action attribute.
By the way, I think this code may be completely broken if formtarget is used.

> It also caused an UNEXPECTED PASS in test_button03.html.

It was a todo which is now fixed with this patch. I will attach a patch removing the "todo".
(Assignee)

Comment 5

8 years ago
Created attachment 449277 [details] [diff] [review]
Patch v1.1

r=sicking

This is removing the todo so the test will not be considered as failing because it passes.

I will write a separate patch for the password manager.
Attachment #445664 - Attachment is obsolete: true
Attachment #445665 - Attachment is obsolete: true
How was this breaking password manager? I saw bug 568779 comment 1 and we should change the .action usage, but if this patch is breaking current tests I should think that's a web-incompatible change.
(Assignee)

Comment 7

8 years ago
Created attachment 449389 [details] [diff] [review]
Fix password manager

We agreed with Justin on IRC this will probably not break the web as it's the behavior required by HTML4 specification.

With this patch, all password manager tests are green.
Attachment #449389 - Flags: review?(dolske)
Compat issues aside, I think it would be unfortunate to require that the passwordmanager code resolve the action attribute against the baseURI itself, so it would be nice to at least preserve some script-exposed way to get the actual action URI. GetActionURL() looks suitable, but I'm not sure what the best way to expose it is... mozActionURL?

(I also wonder why the seemingly-similar GetAction/GetActionURL methods do such different things... some of it is trying to avoid unnecessary string<->URI conversion churn, I guess, but it's confusing at the very least, and potentially worrying for the pwmgr if they can return different values...)
That patch actually highlights part of the confusion - do you really want to use documentURI as the base there, rather than form.baseURI?
Comment on attachment 449389 [details] [diff] [review]
Fix password manager

Yeah, this is wrong. nsHTMLFormElement only uses the documentURI in the action="" case.
Attachment #449389 - Flags: review?(dolske) → review-
(In reply to comment #8)
> so it would be nice to at least preserve some script-exposed way to get the
> actual action URI.

would also have avoided bug 570266!
(Assignee)

Comment 12

7 years ago
Created attachment 466514 [details] [diff] [review]
Patch Part1 v1 - Form action IDL attribute reflects the content attribute

r=sicking
Attachment #449277 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Created attachment 466516 [details] [diff] [review]
Patch Part1 v1.1 - Form action IDL attribute reflects the content attribute

Better with the correct patch...
Attachment #466514 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Created attachment 466518 [details] [diff] [review]
Patch Part2 v1 - Add mozActionURI property

This should help script to be fixed.
Attachment #466518 - Flags: review?(jonas)
(Assignee)

Comment 15

7 years ago
Created attachment 466519 [details] [diff] [review]
Patch Part3 v1 - Fix password manager

This is a simple fix of the password manager using mozActionURI.
Attachment #449389 - Attachment is obsolete: true
Attachment #466519 - Flags: review?(gavin.sharp)
We had a discussion about 'moz' prefixed DOM properties the other day and tantek pointed out that while 'moz' works for us, 'o' doesn't work as well for opera, and so we settled on 'moz_' as prefix.

Also, i've been arguing that using 'uri' is a better name than 'URI' for properties. So would prefer to be consistent with that.

Hence I propose that we name the property:

moz_actionUri
Comment on attachment 466518 [details] [diff] [review]
Patch Part2 v1 - Add mozActionURI property

r=me with the name change.
Attachment #466518 - Flags: review?(jonas) → review+
Wait wait. We've been using the "moz" prefix for lots of DOM stuff already. Let's not break consistency with that.

Opera can use operaFooBar or o_fooBar if they want to. I don't think it really matters if their prefix is inconsistent with ours by containing a _.
Ok, how about mozActionUri then?
Sounds fine to me. I haven't followed the whole uri vs Uri vs URI controversy though.

Note that Webkit has been using webkitFooBar lately.
(Assignee)

Comment 21

7 years ago
Created attachment 466740 [details] [diff] [review]
Patch Part2 v1.1 - Add mozActionUri property

r=sicking

I've changed the name to mozActionUri.
Attachment #466518 - Attachment is obsolete: true
(Assignee)

Comment 22

7 years ago
Blocker request: we want to have bug 566160 in ff4 and this bug is a dependency.
blocking2.0: --- → ?
(Assignee)

Comment 23

7 years ago
Created attachment 466745 [details] [diff] [review]
Patch Part3 v1.1 - Fix password manager
Attachment #466519 - Attachment is obsolete: true
Attachment #466745 - Flags: review?
Attachment #466519 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #466745 - Flags: review? → review?(gavin.sharp)
Attachment #466745 - Flags: review?(gavin.sharp) → review+
Blocks: 570266

Updated

7 years ago
blocking2.0: ? → beta5+
(Assignee)

Comment 24

7 years ago
This is now in the tree.
Changesets:
http://hg.mozilla.org/mozilla-central/rev/4d2aa287de9e
http://hg.mozilla.org/mozilla-central/rev/4f8eb1ef4963
http://hg.mozilla.org/mozilla-central/rev/94e81672bf20
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
(Assignee)

Comment 25

7 years ago
dev-doc may be needed especially for mozAcctionUri
Keywords: dev-doc-needed
Documented:

https://developer.mozilla.org/en/DOM/HTMLFormElement
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 607145
(In reply to comment #26)
> Documented:
> 
> https://developer.mozilla.org/en/DOM/HTMLFormElement

FTR: Bug 607145 removed mozActionUri, so this change needs to be reverted.
(Assignee)

Comment 28

7 years ago
Thanks Jens.
Keywords: dev-doc-complete → dev-doc-needed
Docs re-updated.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.