replace progressmeter XBL binding by a custom element

RESOLVED FIXED in Firefox 64

Status

()

P5
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 months ago
Created attachment 9009005 [details] [diff] [review]
patch
Attachment #9009005 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 1

5 months ago
Comment on attachment 9009005 [details] [diff] [review]
patch

Review of attachment 9009005 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/value/test_progress.xul
@@ +54,5 @@
>      <progressmeter id="pm1" value="50"/>
>      <progressmeter id="pm2" value="500" max="1000"/>
>      <progressmeter id="pm3"/>
> +    <progressmeter id="pm4" mode="undetermined"/>
> +  </vbox>

please ignore these changes, I have removed them locally
(Assignee)

Updated

5 months ago
Assignee: nobody → surkov.alexander

Updated

5 months ago
Priority: -- → P5

Comment 2

5 months ago
Comment on attachment 9009005 [details] [diff] [review]
patch

Can you use "hg mv" here? We're doing this for other bindings as well.

Clearing review for now, I can still look into this version later.
Attachment #9009005 - Flags: review?(paolo.mozmail)

Comment 3

5 months ago
Comment on attachment 9009005 [details] [diff] [review]
patch

Review of attachment 9009005 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/progressmeter.js
@@ +94,5 @@
> +      `;
> +
> +    delete this._fragment;
> +    this._fragment = MozXULElement.parseXULToFragment(content);
> +    this.appendChild(this._fragment.cloneNode(true));

Ah, while I'm here, I noticed you don't need this._fragment and cloneNode if you're not reusing the same fragment for other instances.
Comment on attachment 9009005 [details] [diff] [review]
patch

Review of attachment 9009005 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/progressmeter.js
@@ +94,5 @@
> +      `;
> +
> +    delete this._fragment;
> +    this._fragment = MozXULElement.parseXULToFragment(content);
> +    this.appendChild(this._fragment.cloneNode(true));

We'll need to empty out the DOM in the disconnectedCallback, right? Otherwise if you remove and re-add we'll end up with duplicate content.
(Assignee)

Comment 5

5 months ago
Created attachment 9009504 [details] [diff] [review]
patch
Attachment #9009005 - Attachment is obsolete: true
Attachment #9009504 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 6

5 months ago
(In reply to :Paolo Amadini from comment #3)

> > +    delete this._fragment;
> > +    this._fragment = MozXULElement.parseXULToFragment(content);
> > +    this.appendChild(this._fragment.cloneNode(true));
> 
> Ah, while I'm here, I noticed you don't need this._fragment and cloneNode if
> you're not reusing the same fragment for other instances.

oh, right no cloneNode is needed, otherwise I still need to |delete this._fragment| before creating a new one, correct?

(In reply to Brian Grinstead [:bgrins] from comment #4)
----------------------------------------------------

> > +    delete this._fragment;
> > +    this._fragment = MozXULElement.parseXULToFragment(content);
> > +    this.appendChild(this._fragment.cloneNode(true));
> 
> We'll need to empty out the DOM in the disconnectedCallback, right?
> Otherwise if you remove and re-add we'll end up with duplicate content.

I think you are correct if I do cloneNode on it, but if not, then delete this._fragment should take care of it, no?
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1428869

Updated

5 months ago
Depends on: 1428869

Comment 8

5 months ago
(In reply to alexander :surkov (:asurkov) from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > ::: toolkit/content/widgets/progressmeter.js
> > @@ +94,5 @@
> > > +      `;
> > > +
> > > +    delete this._fragment;
> > > +    this._fragment = MozXULElement.parseXULToFragment(content);
> > > +    this.appendChild(this._fragment.cloneNode(true));
> > 
> > We'll need to empty out the DOM in the disconnectedCallback, right?
> > Otherwise if you remove and re-add we'll end up with duplicate content.
> 
> I think you are correct if I do cloneNode on it, but if not, then delete
> this._fragment should take care of it, no?

No, `delete this._fragment;` does absolutely nothing (apart from adding extra work to the JS engine, which has to remove the `_fragment` property from this element, and then add it again the next operation, both of which are rather expensive operations).

Instead, you need to remove the `delete this._fragment;` call and replace it with `this.textContent = "";` to clear out the DOM.
(Assignee)

Comment 9

5 months ago
Created attachment 9009824 [details] [diff] [review]
patch
Attachment #9009824 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

5 months ago
Attachment #9009504 - Attachment is obsolete: true
Attachment #9009504 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

5 months ago
(In reply to ExE Boss from comment #8)

> Instead, you need to remove the `delete this._fragment;` call and replace it
> with `this.textContent = "";` to clear out the DOM.

right, fixed

Comment 11

5 months ago
Comment on attachment 9009824 [details] [diff] [review]
patch

Review of attachment 9009824 [details] [diff] [review]:
-----------------------------------------------------------------

The conversion looks good, but there is possibly an issue that needs to be addressed on Linux.

::: browser/themes/shared/downloads/progressmeter.inc.css
@@ -15,5 @@
>  
> -.downloadProgress[mode="undetermined"] {
> -  /* for overriding rules on global.css in Linux. */
> -  -moz-binding: url("chrome://global/content/bindings/progressmeter.xml#progressmeter");
> -}

If I understand correctly, you need to find an alternative way to suppress the progress animation on Linux for the Downloads Panel progress bars when the file size is unknown, since we use CSS animations for those on all platforms instead. Maybe use a different attribute than "mode", and update the Downloads Panel styling accordingly?

::: toolkit/content/widgets/progressmeter.js
@@ +73,5 @@
> +  disconnectedCallback() {
> +    this.runAnimation = false;
> +  }
> +
> +  attributeChangedCallback(name, oldValue, newValue) {

Is attributeChangedCallback called if oldValue and newValue are the same? If so, you may need an extra check to avoid doing unnecessary work.

@@ +102,4 @@
>  
> +    this._runAnimation = true;
> +    this._stack = this.querySelector(".progress-remainder");
> +    this._spacer = this.querySelector(".progress-bar");

You need to delete this._stack and this._spacer in the "if (!isUndetermined)" case, so you don't keep references to removed DOM nodes.

@@ +108,2 @@
>  
> +    function nextStep(t) {

You can use an arrow function so you don't need to call "bind" later.
Attachment #9009824 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 12

5 months ago
Created attachment 9010885 [details] [diff] [review]
patch
Attachment #9009824 - Attachment is obsolete: true
Attachment #9010885 - Flags: review?(paolo.mozmail)

Comment 13

5 months ago
Comment on attachment 9010885 [details] [diff] [review]
patch

Review of attachment 9010885 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +318,5 @@
> +      // progress bars when the file size is unknown.
> +      let isLinux =
> +        this.element.ownerGlobal.navigator.platform.includes("Linux");
> +      this.element.setAttribute("progressmode",
> +                                isLinux ? "normal" : "undetermined");

I'll have to check how this looks, since not setting this attribute might influence the special styling we apply for the Downloads Panel.

Also, in front-end modules you can use AppConstants:

AppConstants.platform == "linux" ? "normal" : "undetermined"
(Assignee)

Comment 14

5 months ago
Created attachment 9011357 [details] [diff] [review]
patch

with addressed comments
Attachment #9010885 - Attachment is obsolete: true
Attachment #9010885 - Flags: review?(paolo.mozmail)
Attachment #9011357 - Flags: review?(paolo.mozmail)

Comment 15

5 months ago
Comment on attachment 9011357 [details] [diff] [review]
patch

Review of attachment 9011357 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +318,5 @@
> +      // Suppress the progress animation on Linux for the Downloads Panel
> +      // progress bars when the file size is unknown.
> +      this.element.setAttribute("progressmode",
> +                                AppConstants.platform == "linux" ? "normal" :
> +                                                                   "undetermined");

Sorry for the delay, I had to test on Linux to see how this looked. In fact, this breaks the undetermined progress styling:

https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/browser/themes/shared/downloads/progressmeter.inc.css#32

You can test how this looks by adding a breakpoint or "await new Promise(() => {});" at the end of "browser/components/downloads/test/browser/browser_basic_functionality.js".

I suggest to set "progress" to "100" and change the style above to check a different attribute than "mode".
Attachment #9011357 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 16

5 months ago
Created attachment 9012028 [details] [diff] [review]
patch

thank you for catch, please let me know if I captured your suggestions right (the test looks ok on mac).
Attachment #9011357 - Attachment is obsolete: true
Attachment #9012028 - Flags: review?(paolo.mozmail)

Comment 17

5 months ago
Comment on attachment 9012028 [details] [diff] [review]
patch

Review of attachment 9012028 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/downloads/progressmeter.inc.css
@@ +24,4 @@
>    background-color: GrayText;
>  }
>  
> +.downloadProgress[value="100"] > .progress-bar {

You can't reuse "value" here, otherwise regular progress bars will become animated near the end of the download. You can use a selector like:

.downloadProgress[progressmode="undetermined"] > .progress-bar

An then, in download.xml, change the attribute inheritance:

xbl:inherits="progressmode,value=progress,paused=progresspaused"/>

Above, just set the attributes unconditionally on all platforms:

this.element.setAttribute("progressmode", "undetermined");
this.element.setAttribute("progress", "100");

Setting "progress" to "100" is necessary because the progress bar is now a regular one, and if the value changed previously, then the main bar which we style wouldn't fit the full width.

I'll probably have to change how this works in bug 1452626, but for now this should do the trick.
Attachment #9012028 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 18

5 months ago
Created attachment 9012084 [details] [diff] [review]
patch

(In reply to :Paolo Amadini from comment #17)

> ::: browser/themes/shared/downloads/progressmeter.inc.css
> @@ +24,4 @@
> >    background-color: GrayText;
> >  }
> >  
> > +.downloadProgress[value="100"] > .progress-bar {
> 
> You can't reuse "value" here, otherwise regular progress bars will become
> animated near the end of the download. You can use a selector like:
> 
> .downloadProgress[progressmode="undetermined"] > .progress-bar
> 
> An then, in download.xml, change the attribute inheritance:
> 
> xbl:inherits="progressmode,value=progress,paused=progresspaused"/>

os x relies on on mode="undetermined", at least, it stops working with the change. How about adding a third attribute, something like @progress-undetermined?
Attachment #9012028 - Attachment is obsolete: true
Attachment #9012084 - Flags: review?(paolo.mozmail)

Comment 19

5 months ago
Comment on attachment 9012084 [details] [diff] [review]
patch

Review of attachment 9012084 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I found where the native widget code checks the attributes of the element:

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/widget/nsNativeTheme.cpp#564-580

We also have a special layout frame that apparently modifies the attributes on the inner elements directly:

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/layout/xul/nsProgressMeterFrame.cpp#122-180

I think your solution should work, keeping the mode="undetermined" attribute for platforms other than Linux, and adding the "progress-undetermined" special attribute.

Maybe at some point we'll be able to simplify the look and feel of progress bars and implement them with simple web technologies, and we won't need this special code anymore.

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +315,2 @@
>        this.element.setAttribute("progressmode", "normal");
>        this.element.setAttribute("progress", this.download.progress);

Remove the progress-undetermined attribute in this other branch.

::: toolkit/content/widgets/progressmeter.js
@@ +4,4 @@
>  
> +"use strict";
> +
> +{

There's a new comment we add before this opening brace, you can copy it from other Custom Element definition files.
Attachment #9012084 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 20

5 months ago
Created attachment 9013922 [details] [diff] [review]
patch

with addressed comments
Attachment #9012084 - Attachment is obsolete: true

Comment 21

5 months ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5543e7f5ddad
replace progressmeter XBL binding by a custom element, r=paolo

Comment 22

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5543e7f5ddad
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1496189

Updated

5 months ago
Depends on: 1496861
Depends on: 1497599
Depends on: 1497837
Depends on: 1497975

Updated

4 months ago
Depends on: 1498678
Depends on: 1499843
You need to log in before you can comment on or make changes to this bug.