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

feature(parser/render): Multiple roles for images. #669

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

gdamore
Copy link
Collaborator

@gdamore gdamore commented Jun 30, 2020

This also includes bug fixes for parser. We've tried to
emulate most, but not all, of the quirks found in asciidoctor
with respect to images.

(Following commentary not in the git log:)

This PR refactors (and removes) some of the logic intended to make verseblocks and quoteblocks work like asciidoctor. Apparently asciidoctor treats [quote] differently from [verse] when process image blocks. Both of these cases are actually pathological, since imageblocks should never be the content of a verse or a quote (the result was actually undesirable as well -- leaving source markup in the output). In order to bring some sanity, we treat them both the same, and assume that any attributes ahead of an image:: block really are meant to apply to the image.

We also noted that the handling for inline attributes vs. block attributes is inconsistent, in strange ways. Both use the positional parameter as the "alt" for the image, but inline does not permit shorthand role or ID elements (I think this was noticed as a bug that I filed against asciidoctor), and the 2nd and 3rd positional parameters for height and width are only honored in the inline attributes (in block attributes they are ignored.)

Trying to find some sanity here, we opted to consistently treat the 2nd and 3rd parameters as width and height, regardless of where they are. We do treat the alt tag (first positional parameter) differently as that seemed like it might break existing documents if we did otherwise (though documents that rely on that might be considered defective). Note that as a consequence of this, it is impossible to apply multiple roles to an inline image. (We would encourage fixing this, and making inline attributes more strictly follow the block attribute list rules -- that can be a follow up PR.)

There is some strangeness around the acceptance or not of whitespace in attribute lists. The grammar fixes here are meant to make us conform more tightly to what asciidoctor does, but again it's not 100%. The places where differ are very pathological -- and easily worked around.

@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #669 into master will decrease coverage by 0.49%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #669      +/-   ##
==========================================
- Coverage   85.66%   85.17%   -0.50%     
==========================================
  Files          71       71              
  Lines        4715     4721       +6     
==========================================
- Hits         4039     4021      -18     
- Misses        422      446      +24     
  Partials      254      254              

This also includes bug fixes for parser.  We've tried to
emulate most, but not all, of the quirks found in asciidoctor
with respect to images.
}

ImageAttributes <- "[" alt:(StandaloneAttributeValue)? ","? width:(StandaloneAttributeValue)? ","? height:(StandaloneAttributeValue)? ","? Space* otherattrs:(GenericAttribute)* "]" {
return types.NewImageAttributes(alt, width, height, otherattrs.([]interface{}))
ImageAltAttr <- Space* value:PositionalValue Space* {
Copy link
Member

Choose a reason for hiding this comment

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

I assume that the support for spaces around the positional value in the ImageAltAttr rule vs lack of support for spaces in the ImageAltAttrInline rule is related to the comment below found in the PR?

There is some strangeness around the acceptance or not of whitespace in attribute lists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You would assume correctly. I am trying hard not to describe all the ways in which asciidoctor lacks consistency.

Copy link
Member

Choose a reason for hiding this comment

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

yes, that one more area in which I hope the WG will clarify!

@xcoulon
Copy link
Member

xcoulon commented Jun 30, 2020

thanks a lot for all the investigation and all the work in this PR, @gdamore! 🙌

@xcoulon xcoulon merged commit efdeeea into bytesparadise:master Jun 30, 2020
@gdamore gdamore deleted the imageblock branch July 4, 2020 16:54
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