566128 - HTMLFormElement action IDL attribute should reflect the action content attribute

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

|GetURIAttr| should be done outsite of |GetAction|, in other words, it should be done by callers if needed.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Tests v1

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Mounir Lamouri (:mounir)]

(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".

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: 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.

Comment 6

[Justin Dolske [:Dolske]]

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.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: 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.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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...)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

That patch actually highlights part of the confusion - do you really want to use documentURI as the base there, rather than form.baseURI?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 449389 [details] [diff] [review]
Fix password manager

Yeah, this is wrong. nsHTMLFormElement only uses the documentURI in the action="" case.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(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!

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Patch Part1 v1 - Form action IDL attribute reflects the content attribute

r=sicking

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch Part1 v1.1 - Form action IDL attribute reflects the content attribute

Better with the correct patch...

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch Part2 v1 - Add mozActionURI property

This should help script to be fixed.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch Part3 v1 - Fix password manager

This is a simple fix of the password manager using mozActionURI.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

r=me with the name change.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 _.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, how about mozActionUri then?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch Part2 v1.1 - Add mozActionUri property

r=sicking

I've changed the name to mozActionUri.

Comment 22

[Mounir Lamouri (:mounir)]

Blocker request: we want to have bug 566160 in ff4 and this bug is a dependency.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch Part3 v1.1 - Fix password manager

Comment 24

[Mounir Lamouri (:mounir)]

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

Comment 25

[Mounir Lamouri (:mounir)]

dev-doc may be needed especially for mozAcctionUri

Comment 26

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/DOM/HTMLFormElement

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

(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.

Comment 28

[Mounir Lamouri (:mounir)]

Thanks Jens.

Comment 29

[Eric Shepherd [:sheppy]]

Docs re-updated.