Closed Bug 842500 Opened 11 years ago Closed 11 years ago

"TypeError: Floating-point value is not finite." when assigning undefined to a <progress> value

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g18 fixed, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected)

RESOLVED FIXED
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(1 file)

In the App Install Manager, we use the following code to make a progress node indeterminate :

progressNode.value = undefined;

In Firefox Nightly (21) this is now forbidden and it fails with :

     TypeError: Floating-point value is not finite.
      at ai_handleProgress (http://system.gaiamobile.org:8080/js/app_install_manager.js:308)

This is not clear how this should be done instead. This is most probably related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so I'm CCing Mounir on this.

Note also that I find very strange that to know that a progress bar is indeterminate, we ask for (node.position === -1) but to set a progress bar indeterminate we have to fiddle with the value attribute.
Note: this is not a problem now because we use a gecko18 branch but this will be as soon as we change the branch. For example, merely running the unit tests in Fx nightly triggers this.
Looks like a regression from bug 839033. We should probably make those unrestricted for now... Mounir?
Blocks: 839033
Flags: needinfo?(mounir)
(In reply to Julien Wajsberg [:julienw] from comment #0)
> In the App Install Manager, we use the following code to make a progress
> node indeterminate :
> 
> progressNode.value = undefined;

Are you doing that instead of progressNode.removeAttribute('value'); because it is easier to write?

> This is not clear how this should be done instead. This is most probably
> related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so

What's your opinion on that?

> Note also that I find very strange that to know that a progress bar is
> indeterminate, we ask for (node.position === -1) but to set a progress bar
> indeterminate we have to fiddle with the value attribute.

Would you prefer a 'indeterminate' attribute?
Flags: needinfo?(mounir)
(In reply to :Ms2ger from comment #2)
> Looks like a regression from bug 839033. We should probably make those
> unrestricted for now... Mounir?

They can simply call .removeAttribute('value') to work around this issue. I am not sure if we should change our behaviour now and hope that the specification will follow. In https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 Hixie didn't express a lot of interest in that (the fact that .undefined isn't allowed is highly related with that spec bug).
(In reply to Mounir Lamouri (:mounir) from comment #3)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > In the App Install Manager, we use the following code to make a progress
> > node indeterminate :
> > 
> > progressNode.value = undefined;
> 
> Are you doing that instead of progressNode.removeAttribute('value'); because
> it is easier to write?

Because I felt awkward to modify the DOM whereas I wanted to change the object's state. Plus I thought it was equivalent (but the spec is really not clear on how DOM attribute values or absence translates to IDL).

> 
> > This is not clear how this should be done instead. This is most probably
> > related to the issue https://www.w3.org/Bugs/Public/show_bug.cgi?id=11937 so
> 
> What's your opinion on that?

The bug does not really deal with setting value from JavaScript. About handling invalid value specified in markup, I don't mind either way. But it should be valid to set "undefined" to the IDL-reflected property, which would mean as if the attribute is removed in the DOM. Maybe the rules [1] should be changed to have the following as first rule: "if 'undefined' is set to the property, remove the attribute".

> 
> > Note also that I find very strange that to know that a progress bar is
> > indeterminate, we ask for (node.position === -1) but to set a progress bar
> > indeterminate we have to fiddle with the value attribute.
> 
> Would you prefer a 'indeterminate' attribute?

As a read attribute it could be nice but (position === "-1") is good too. But it would also be consistent that getting the value would return |undefined|. Although I don't mind as soon as this is clearly specified, and I believe getting values is correctly specified now, only setting values is not clear and/or logical.

[1] http://www.w3.org/html/wg/drafts/html/master/infrastructure.html#best-representation-of-the-number-as-a-floating-point-number
Attached patch patch v1Splinter Review
- use removeAttribute instead of assigning undefined
---
 apps/system/js/app_install_manager.js |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The tests are passing again in nightly. I tried installing various apps, packaged or not, with or without a size, and this works correctly. Thus asking for approval too because otherwise tests will fail on current nightly, and this file is otherwise very well tested.
Assignee: nobody → felash
Attachment #719611 - Flags: review?(etienne)
Attachment #719611 - Flags: approval-gaia-v1?(21)
Attachment #719611 - Flags: review?(etienne) → review+
master: https://github.com/mozilla-b2g/gaia/commit/95d47f725af83e701aefa694d1a90ae7fe8a5d95
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #719611 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
Uplifted commit 95d47f725af83e701aefa694d1a90ae7fe8a5d95 as:
v1-train: 26f9f0aac31dba983dfde1e94a7be18cd6887de8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: