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

stop removal of spaces that causes display changes #150

Merged

Conversation

tisdall
Copy link
Contributor

@tisdall tisdall commented Jan 15, 2021

I can't believe how much time I spent on this... 😄

This PR fixes some additional cases of removal of spaces that changes the display. Most of the cases are poorly formed HTML, but unfortunately we're not always in control over that. In some edge cases it's leaving a single space that doesn't affect display in the browsers I tested, but I think this keeps things simple and errs on the side of leaving a space that may affect display. I looked further into #121 and #21 and neither of them are actually bugs in the latest version.

Examples

(I'm going to exclude the <html> wrappers in the output for simplicity)

Example 1:

Stop removal of needed space for proper display.

Un-minified: <p> <i>first </i>second </p> which displays as: first second
django-htmlmi 0.11.0: <p><i>first</i>second</p> which displays as: firstsecond
this PR: <p><i>first </i>second</p> which displays as: first second

Example 2:

Stop the removal of a space that has styling applied to it (hard to duplicate in github, but you can see the space is underlined between the words)

Un-minified: <p><u>first </u> <s> second</s></p> which displays as: first second
django-htmlmi 0.11.0: <p><u>first</u> <s>second</s></p> which displays as: first second
this PR: <p><u>first </u> <s> second</s></p> which displays as: first second

This one is weird as the order dictates what styling that space in the middle receives. If you switch the order then the space in the middle gets a strike-through styling, but I have no idea if that's consistent across all browser renderers.

Copy link
Member

@andrewsmedina andrewsmedina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing. There are few things that can be improved and there is a test broken that should be fixed.

Comment on lines +189 to +191
if is_text_tag(soup.parent):
# NavigableString is within a text tag element
return (True, True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function always returns the same value. Why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially, this is an optimization. Prior to this, I had something like this:

prev_flow = (
    is_text_tag(soup.previous_sibling)
    or is_text_tag(soup.parent)
)

next_flow = (
    is_text_tag(soup.next_sibling)
    or is_text_tag(soup.parent)
)

Then I noticed that I was essentially running is_text_tag(soup.parent) twice and that when True it forces both prev_flow and next_flow to True.

I'm open to suggestions on a code comment to make it more understandable.

htmlmin/minify.py Outdated Show resolved Hide resolved
@tisdall tisdall force-pushed the keep_needed_inline_text_spaces branch from 988be35 to 25b0332 Compare January 18, 2021 13:16
@tisdall
Copy link
Contributor Author

tisdall commented Jan 18, 2021

I must have accidentally hit space while having the test_minify.py open or something as I had run the tests locally. I've updated that one test and made the suggested change and updated the PR.

Copy link
Member

@andrewsmedina andrewsmedina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewsmedina andrewsmedina merged commit afcb214 into cobrateam:master Jan 20, 2021
@tisdall tisdall deleted the keep_needed_inline_text_spaces branch January 20, 2021 13:40
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.

2 participants