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

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.

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.

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

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

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

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"

with addressed comments

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

thank you for catch, please let me know if I captured your suggestions right (the test looks ok on mac).

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.

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

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.

with addressed comments

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