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.

Sign up for GitHub

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

merge 'class' attr when both img and svg specify it #9653

Merged
merged 5 commits into from
Apr 15, 2024
Merged

merge 'class' attr when both img and svg specify it #9653

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0421a1
merge 'class' attr when both img and svg specify it
cscheid Apr 12, 2024
8e209e2
command test for #9652
cscheid Apr 12, 2024
7590aa9
fix duplicate 'class'
cscheid Apr 12, 2024
2fd0564
use a second comprehension for imgAttrs
cscheid Apr 15, 2024
2aaead5
use a second comprehension for imgAttrs
cscheid Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/Text/Pandoc/SelfContained.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,10 @@ combineSvgAttrs svgAttrs imgAttrs =
dropPointZero t = case T.stripSuffix ".0" t of
Nothing -> t
Just t' -> t'
combinedAttrs = concat [
imgAttrs,
combinedAttrs = replaceMaybe mergedClasses $ imgAttrs ++
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of using replaceMaybe I'd suggest getting rid of that function and just added "class" to the list at ln. 264 and also use [kv | (k,v) <- imgAttrs | k /= "class"].

But isn't there another possible source of duplicated attributes? For example, suppose imgAttrs contains widdth and so does svgAttrs; won't we get two width attributes?

Copy link
Contributor Author

@cscheid cscheid Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of using replaceMaybe I'd suggest getting rid of that function and just added "class" to the list at ln. 264 and also use [kv | (k,v) <- imgAttrs | k /= "class"].

Thanks - my Haskell is (very visibly) rusty, and I forgot about comprehensions.

But isn't there another possible source of duplicated attributes? For example, suppose imgAttrs contains widdth and so does svgAttrs; won't we get two width attributes?

Yes, I believe we will. I had a narrow fix in mind for the class attribute since the other ones don't seem to have a natural way to merge the data. I can make further changes, but I don't know what you would prefer:

  • let imgAttrs attribute win
  • let svgAttrs attribute win
  • issue a warning
  • issue an error

The current behavior (duplicating width attribute) appears to not cause an error on either Google Chrome or Firefox, and the first attribute wins: <svg width="200" width="300"></svg> becomes a DOM element with width="200". With that said, I don't know if this is enough to justify inaction in your view (and I don't know if there's a spec that enforces this).

[(k, v) | (k, v) <- svgAttrs
, isNothing (lookup k imgAttrs)
, k `notElem` ["xmlns", "xmlns:xlink", "version"]],
mergedClasses]
, k `notElem` ["xmlns", "xmlns:xlink", "version"]]
parseViewBox t =
case map (safeRead . addZero) $ T.words t of
[Just llx, Just lly, Just urx, Just ury] -> Just (llx, lly, urx, ury)
Expand All @@ -277,11 +275,12 @@ combineSvgAttrs svgAttrs imgAttrs =
(mbHeight :: Maybe Int) = lookup "height" combinedAttrs >>= safeRead
(mbWidth :: Maybe Int) = lookup "width" combinedAttrs >>= safeRead
-- https://github.com/jgm/pandoc/issues/9652
imgClasses = lookup "class" imgAttrs
svgClasses = lookup "class" svgAttrs
mergedClasses = case (imgClasses, svgClasses) of
(Just c1, Just c2) -> [("class", c1 <> " " <> c2)]
_ -> []
mergedClasses = case (lookup "class" imgAttrs, lookup "class" svgAttrs) of
(Just c1, Just c2) -> Just ("class", c1 <> " " <> c2)
_ -> Nothing
replaceMaybe :: Eq a => Maybe (a, b) -> [(a, b)] -> [(a, b)]
replaceMaybe Nothing = id
replaceMaybe (Just x) = map (\pair@(a, _) -> if a == fst x then x else pair)

cssURLs :: PandocMonad m
=> FilePath -> ByteString -> m ByteString
Expand Down
2 changes: 1 addition & 1 deletion test/command/9652.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
<img class="something" src="9652.svg" />
```
^D
<svg id="svg_b627f92299158b36552b" role="img" class="something" width="504.00pt" height="360.00pt" viewBox="0 0 504.00 360.00" class="something please-do-not-delete-me">
<svg id="svg_b627f92299158b36552b" role="img" class="something please-do-not-delete-me" width="504.00pt" height="360.00pt" viewBox="0 0 504.00 360.00">
</svg>
````
Loading