Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix computing video view's width and height when using padding #89

Merged
merged 2 commits into from
Nov 22, 2015

Conversation

saghul
Copy link
Collaborator

@saghul saghul commented Nov 20, 2015

According to the MDN docs the padding is included, so substract it.

@saghul
Copy link
Collaborator Author

saghul commented Nov 20, 2015

/cc @ibc

@hthetiot
Copy link
Contributor

lgtm

@ibc
Copy link
Collaborator

ibc commented Nov 22, 2015

Question: how does this work? The video width/height is modified based on the CSS padding, but I do not see the X/Y position of the video being updated. Do I miss something?

@ibc
Copy link
Collaborator

ibc commented Nov 22, 2015

I mean: do elementLeftetc properly point to the position of the video?

@ibc
Copy link
Collaborator

ibc commented Nov 22, 2015

To be clear:

elementLeft is the left value retrieved from:

function getElementPositionAndSize() {
    var rect = this.element.getBoundingClientRect();

    return {
        left:   rect.left + this.element.clientLeft,
        top:    rect.top + this.element.clientTop,
        width:  this.element.clientWidth,
        height: this.element.clientHeight
    };
}

My concern is: If a <video> element has padding, where is left points to?

getBoundingClientRect() "returns a TextRectangle object that includes the padding, scrollbar, and the border, but excludes the margin":

That's the reason of my worry.

@saghul
Copy link
Collaborator Author

saghul commented Nov 22, 2015

Yep, forgot to take care of that. Will test and fix.
On Nov 22, 2015 5:50 PM, "Iñaki Baz Castillo" [email protected]
wrote:

To be clear:

elementLeft is the left value retrieved from:

function getElementPositionAndSize() {
var rect = this.element.getBoundingClientRect();

return {
    left:   rect.left + this.element.clientLeft,
    top:    rect.top + this.element.clientTop,
    width:  this.element.clientWidth,
    height: this.element.clientHeight
};

}

My concern is: If a

getBoundingClientRect() "returns a TextRectangle object that includes the
padding, scrollbar, and the border, but excludes the margin":

That's the reason of my worry.


Reply to this email directly or view it on GitHub
#89 (comment)
.

@saghul
Copy link
Collaborator Author

saghul commented Nov 22, 2015

@ibc Fixed and tested, can you PTAL?

@ibc
Copy link
Collaborator

ibc commented Nov 22, 2015

Cannot check it now, but:

Padding is not added to te CSS width/height when box-sizing is "border-box" and may be other values, so this would need even more work:

https://developer.mozilla.org/es/docs/Web/CSS/box-sizing

@saghul
Copy link
Collaborator Author

saghul commented Nov 22, 2015

box-sizing is not currently supported by the plugin AFAIK, so that would be another patch/improvement IMHO. This patch just adds support for padding-* to the current set of supported properties.

ibc added a commit that referenced this pull request Nov 22, 2015
Fix computing video view's width and height when using padding
@ibc ibc merged commit 502d5a1 into cordova-rtc:master Nov 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants