ProgressBar: message appears twice when progressBar has an initial value #551

EAlexRojas · 2015-04-16T09:56:31Z

When a ProgressBar is created with an initial value the message appears in two different places:

The width of msgInvertNode (white text) is changed in refeshRendering method in order to have the same width than msgNode using getComputedStyle. (here)

This works on 0.6.0.

I found that on 0.6.0 version, refreshRendering is called when there's already something displayed on the screen, but on actual version, when refreshRendering is called there's nothing displayed yet on the screen. (so, getComputedStyle doesn't returns the desired value)

Not sure if it's a delite problem or it's suposed to be like that and so it's a progressBar problem.

@wkeese what do you think about it?

Code used:

<d-progress-bar value=40 max=100></d-progress-bar>

wkeese · 2015-04-16T19:32:37Z

This needs to be fixed in ProgressBar. refreshRendering() shouldn't assume that the widget is attached to the document. That's what attachedCallback() is for.

I guess you are talking about this code:

if ("value" in props || "max" in props) {
    if (this.position === -1) { //indeterminate state
        this.indicatorNode.style.removeProperty("width");
        this.removeAttribute("aria-valuenow");
    } else {
        this.indicatorNode.style.width = (this.position * 100) + "%";
        this.msgInvertNode.style.width =
            window.getComputedStyle(this.msgNode).getPropertyValue("width");
        this.setAttribute("aria-valuenow", this.value);
    }
}

I don't understand why it's calling this.indicatorNode.style.removeProperty("width"); when position === -1, but if that's really needed, it makes things more complicated. Perhaps attachedCallback() can set the width, and refreshRendering() can toggle style.display between block and none.

wkeese · 2015-04-17T06:55:19Z

PS: I got confused in my last comment between indicatorNode and msgInvertNode. It seems like all you need to do to fix this problem is to move:

        this.msgInvertNode.style.width =
            window.getComputedStyle(this.msgNode).getPropertyValue("width");

to attachedCallback()?

EAlexRojas · 2015-04-17T13:25:06Z

Even doing that on attachedCallback I have the same result.

Based on the samples, I'm using this code to test:

...
    <script type="text/javascript">
        require([
            "deliteful/ProgressBar",
            "delite/theme!delite/themes/{{theme}}/global.css",  // page level CSS
            "requirejs-domready/domReady!"
        ], function () {
            document.body.style.display = "";
        });
    </script>
...
<body style="display: none">
    <d-progress-bar value=40 max=100></d-progress-bar>
</body>

The problem I found is that this:

window.getComputedStyle(this.msgNode).getPropertyValue("width");

returns 100% if progressBar is not displayed yet and a value in px if progressBar is already displayed.

Given the progressBar structure:

<d-progress-bar ...>
    <div class="d-progress-bar-background">
        <div class="d-progress-bar-msg">40%</div>
        <div class="d-progress-bar-indicator" style="width: 40%;">
            <div class="d-progress-bar-msg-invert" style="width: 100%;">40%</div>
            <div class="d-progress-bar-a11y"></div>
        </div>
    </div>
</d-progress-bar>

That value in px is needed to ensure that msgInvertNode has the same width than msgNode and so, the two message will be in the same place. (However this cause the problem described in #445)

[I found that refreshRendering is also called after body is displayed... so, If on refreshRendering, the msgInvertNode width is always updated (not only if "value" or "max" are in the props), It works. at least as on 0.6.0 version. But I'm not sure if it's a good approach.]

wkeese · 2015-04-17T23:25:06Z

Ah I see. Well, the root issue is that ProgressBar is doing sizing in javascript. It shouldn't be doing this, and actually in https://docs.google.com/document/d/1Sa0rn4udqO_ea20o4xxogRP6P6ZLgHPMEj4A2V4g7hM/edit#heading=h.tbbul8ly7vli we wrote that:

making sure our components leverage CSS as much as possible, do not rely on JavaScript for layout/styling purposes.

Presumably ProgressBar never really fully worked. For example, if the ProgressBar is in the hidden pane of a SwapView and then that pane is later displayed (after ProgressBar's getComputedStyle() call). Or if there is a >0ms delay between when the ProgressBar is instantiated and when it is attached to the document, causing the getComputedStyle() to occur before the ProgressBar is attached.

You could work around the current test failures by either:

using a this.defer(..., 10) call in attachedCallback()
initially set <body>'s style to visibility:hidden rather than display:none

Neither of those though will address the bigger issue. I think the proper solution to this problem would use clipping (https://css-tricks.com/clipping-masking-css/). If clipping works then ISTM your generated markup could be as simple as:

<d-progress-bar style="width: 200px; position: relative; display: block;">
    <div style="position: absolute; width: 100%; height: 100%; background: blue; color: white; clip: rect(0px 10px 50% 200px)">50%</div>
    50%
</d-progress-bar>

That doesn't quite work though. I couldn't get clip to work with percentages, and for some reason couldn't get clip-path to work at all, even on Chrome. Perhaps it can be done use <clipPath> in SVG.

cjolif · 2015-04-18T09:04:22Z

Obviously it would be better to make the ProgressBar work without JavaScript layout, however I don't think we have the bandwidth for that right now.

Also it is not because we said that we should use CSS and not JavaScript for layout purpose when we can that we shouldn't have a story about how it should be done when CSS is not a possible option (I'm not talking about this case but more generally, there will definitely be such cases like charting for example).

So I think we should be able to tell @EAlexRojas how to modify ProgressBar itself to make it back working with the JavaScript layout. If not this means we don't cover the 2nd use-case we said we want to avoid as much as possible but we never said we will never have?

tkrugg · 2015-04-22T12:36:23Z

We might be able to get rid of the computed style by playing with the positioning, so that the inverted message scales to width:100% of the grand parent rather than the immediate parent. I'll let @EAlexRojas explore that possibility.

EAlexRojas · 2015-04-22T13:30:28Z

I tried using positioning to achieve that and effectively the inverted message can be scaled to 100% of the grand parent, but that brakes the overflow:hidden so the inverted message hides the black message.

EAlexRojas · 2015-04-24T12:27:07Z

Another way to get rid of the getComputedStyle() is using a css class to define the width of the progress bar and using the same css class on the msgInvertNode.

However a default width should be needed and the width could not be specified in %.

cjolif · 2015-05-28T13:59:51Z

Moving to low as we have a workaround.

cjolif added the bug label Apr 24, 2015

EAlexRojas added a commit to EAlexRojas/deliteful that referenced this issue May 6, 2015

ProgressBar: Change on sample to work around ibm-js#551

16c4978

EAlexRojas mentioned this issue May 6, 2015

ProgressBar: Changes on sample to work around #551 #561

Merged

cjolif added the 1-low label May 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ProgressBar: message appears twice when progressBar has an initial value #551

ProgressBar: message appears twice when progressBar has an initial value #551

EAlexRojas commented Apr 16, 2015

wkeese commented Apr 16, 2015

wkeese commented Apr 17, 2015

EAlexRojas commented Apr 17, 2015

wkeese commented Apr 17, 2015

cjolif commented Apr 18, 2015

tkrugg commented Apr 22, 2015

EAlexRojas commented Apr 22, 2015

EAlexRojas commented Apr 24, 2015

cjolif commented May 28, 2015

ProgressBar: message appears twice when progressBar has an initial value #551

ProgressBar: message appears twice when progressBar has an initial value #551

Comments

EAlexRojas commented Apr 16, 2015

wkeese commented Apr 16, 2015

wkeese commented Apr 17, 2015

EAlexRojas commented Apr 17, 2015

wkeese commented Apr 17, 2015

cjolif commented Apr 18, 2015

tkrugg commented Apr 22, 2015

EAlexRojas commented Apr 22, 2015

EAlexRojas commented Apr 24, 2015

cjolif commented May 28, 2015