ProgressBar: message appears twice when progressBar has an initial value

EAlexRojas commented 9 years ago

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

progressbar

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 https://github.com/ibm-js/deliteful/issues/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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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. pb40 pb50

EAlexRojas commented 9 years ago

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 commented 9 years ago

Moving to low as we have a workaround.

ibm-js / deliteful

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