ElementClipEdge descriptor (addresses #21)

woldie commented 8 years ago

This is a work in progress, I've got more tests to write for "auto" in clip and test weirdo corner cases, but it's mostly working. I've tested what I've got on IE8 and Firefox, would appreciate knowing what I've got works on all your other required browsers.

Addresses #21

Todos for this PR:

[x] Get rid of the JSDoc added in the initial commits
[x] Write tests to check for position: absolute or fixed and make sure assertions against clip fail when position is something else
[x] Write tests that prove clip assertions are working as intended
[x] Assert clip against "auto" to test if clip is disabled on an element
[x] Try the forcing reflow thingy Jim suggested for quixote.js that was failing on my sluggy dev box

jamesshore commented 8 years ago

Thanks for this. clip is a CSS property I'm not very familiar with, so it will take a me a little while to review this in detail. So here's some style issue that jumped out at me instead. Sorry if this seems nitpicky and missing the point, like I said, it will take a little while to get into the substance of it.

Please create an issue explaining the feature that we're adding and its motivation and reference this pull request. Then rename the pull request to describe what it adds and the issue (for example, "ElementClipEdge descriptor (addresses issue #xx)
The code in getRawClipStyle() should live in ElementClipEdge, not QElement. The purpose of descriptors is to take raw style information and turn it into useful "cooked" results. That's exactly what getRawClipStyle is doing, so it belongs in the descriptor.
Generally, throwing an exception should be avoided. Would it be correct to say that, if the clip property isn't set, then an element is "clipped" to its full size? If that's true, would it make sense to fall back to the element edge when the property is missing?
I'm not using JSDoc at this time, so documentation should be put in the .md files in the docs/ directory. I'm open to changing that in the future but it needs to be one or the other. :-) Descriptor documentation goes in docs/descriptors.md. Only the public API is documented.
Regarding the detectFontAlignment fix, using setInterval is a clever trick. I can't reproduce the issue you faced on my end, but I'd like to try something simpler... can you try some code for me and see if it works? This code forces a reflow rather than using an interval:

function detectFontEnlargement(frame, frameWidth, callback) {
    ensure.that(frameWidth >= 1500, "Detector frame width must be larger than screen to detect font enlargement");

    // WORKAROUND IE 8: we use a <div> because the <style> tag can't be added by frame.add(). At the time of this
    // writing, I'm not sure if the issue is with frame.add() or if IE just can't programmatically add <style> tags.
    frame.add("<div><style>p { font-size: 15px; }</style></div>");

    var text = frame.add("<p>arbitrary text</p>");
    frame.add("<p>must have two p tags to work</p>");

    // WORKAROUND IE 8: need to force reflow or getting font-size may fail below
    var forceReflow = domElement.offsetHeight;

    // WORKAROUND Safari 8.0.0: timeout required because font is enlarged asynchronously
    setTimeout(function() {
        var fontSize = text.getRawStyle("font-size");
        ensure.that(fontSize !== "", "Expected font-size to be a value");

        // WORKAROUND IE 8: ignores <style> tag we added above
        if (fontSize === "12pt") return callback(false);

        return callback(fontSize !== "15px");
    }, 0);

}

jamesshore commented 8 years ago

Just looked at the MDN page and realized that clip is deprecated. :-( Not sure if it's a good idea to support deprecated properties...

woldie commented 8 years ago

Please create an issue explaining the feature that we're adding and its motivation and reference this pull request. Then rename the pull request to describe what it adds and the issue (for example, "ElementClipEdge descriptor (addresses issue #xx)

That's fine, I'll put that together this week.

The code in getRawClipStyle() should live in ElementClipEdge, not QElement. The purpose of descriptors is to take raw style information and turn it into useful "cooked" results. That's exactly what getRawClipStyle is doing, so it belongs in the descriptor.

Ok, do you want me to move getRawClipStyle() itself as well or just leave that one in QElement and move all of its support code to ElementClipEdge?

Generally, throwing an exception should be avoided. Would it be correct to say that, if the clip property isn't set, then an element is "clipped" to its full size? If that's true, would it make sense to fall back to the element edge when the property is missing?

Heh, I wondered what you'd think about that. So, some of the rules for clip are: it does not cascade, it only applies to position: absolute or fixed, and the default setting if you haven't assigned it a style is clip: auto. If you have clip: auto on an element, then the element is NOT clipped. If you assign a rect to clip, then the edge values in clip can be "auto". "clip: rect(auto auto auto auto)" is legal and that would set the clipping bounds to the border edges of the element.

A clip edge can be: unset, auto, or a css length value. auto can be evaluated to a css length value, so really we have unset and css length. We can have some special mode on the Position object to represent an "un-position", but I just preferred to throw because the caller should know better than try to access a clip that isn't set.

I'm not using JSDoc at this time, so documentation should be put in the .md files in the docs/ directory. I'm open to changing that in the future but it needs to be one or the other. :-) Descriptor documentation goes in docs/descriptors.md. Only the public API is documented.

Yeah, it was just for my sanity while I was trying to figure out how everything worked. I can remove all those. JSDoc is really nice for the suggestions/inspections in the IDE though, you should retrofit quixote with it someday.

Regarding the detectFontAlignment fix, using setInterval is a clever trick. I can't reproduce the issue you faced on my end, but I'd like to try something simpler... can you try some code for me and see if it works? This code forces a reflow rather than using an interval:

I'll give it a shot this week, that sounds very plausible.

woldie commented 8 years ago

Just looked at the MDN page and realized that clip is deprecated. :-( Not sure if it's a good idea to support deprecated properties...

Hehe, yeah, but look at the replacement: http://caniuse.com/#feat=css-clip-path Not a single green box in the whole shooting match. clip-path will be amazing because you can do arbitrary shapes to mask off the element, like circles and paths. The deprecation will make more sense once clip-path is implemented fully on all the evergreen browsers. Right now, it feels a bit like flexbox.

I'm not saying clip is my favorite CSS 2.1 feature, but it's universally implemented correctly even on extreme legacy IE. There's simply no way any browser could remove support for it, like, ever or they'd break compatibility with a jillion sites. I think it is a really interesting feature of CSS, and it even got a bit of modernization recently - clip can be the target of css-animations! Quixote would surely tilt towards this windmill.

jamesshore commented 8 years ago

do you want me to move getRawClipStyle() itself as well or just leave that one in QElement and move all of its support code to ElementClipEdge?

All of it should go in ElementClipEdge.

JSDoc is really nice for the suggestions/inspections in the IDE though, you should retrofit quixote with it someday.

I'm open to the option. Feel free to create an issue for it. :-) We can discuss how it would have to work.

woldie commented 8 years ago

Going to start some fresh PR's for the visible descriptor

jamesshore / quixote

ElementClipEdge descriptor (addresses #21) #20