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

Corrections to width calculations. #356

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexgo91
Copy link

Corrections to width calculations: handled some missing cases + corrected some existing widths.

  1. getCalculatedWidth: 'display:block' elements will lookup at parent's calculated width, instead of solely looking at 'width' attribute.
  2. getCalculatedWidth: HtmlImage's width will be calculated according to image dimensions (it was defaulting to the 'else' part, always returning 0 via getContentWidth()).
  3. getContentWidth: child with 'display:none' will have width 0.
  4. getContentWidth: 'script' tags will have width 0 (previously it was calculated according to their text content).
  5. getContentWidth: before proceeding to checking content's length, we'll check the 'style=width:X' attribute - if it exists then this is our width.
  6. getContentWidth: HtmlImage's width will be calculated according to image dimensions (previously it was calculated by length of text content).
  7. getContentWidth: HTMLElement's content width must be calculated with getContentWidth(). getCalculatedWidth() will not work correctly for inline-block calculations.
  8. getContentWidth: Text elements should be trimmed when calculating content's width - this is how browsers handle text. Llimitation: there is currently no way to differentiate between   tags and actual space.
  9. getContentWidth: spaces/indentation between tags are sometimes rendered as Text nodes, consisting of whitespace. Solution in bullet 8 will prevent these from adding length to width calculations (important!)
  10. getLeft: should rely on parent's calculated length, not solely on parent's 'width' attribute (requires further testing).

@rbri
Copy link
Member

rbri commented May 30, 2021

Hi Alex,
many thanks for this contribution.

It will be great, if you can add some tests cases.

@alexgo91
Copy link
Author

Hey rbri,

I have added several test cases, thank you 👍

@rbri
Copy link
Member

rbri commented Jun 24, 2021

I'm already working on this - will do this slightly different so no direct merge. Please be a bit patient, changes are on the way.

@rbri
Copy link
Member

rbri commented Nov 13, 2021

all your tests are added, have done some changes to pass most of them.

@alexgo91
Copy link
Author

Alright thank you for the update rbri 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants