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

auto-sizing circumgraph image in chrome #38

Closed
aphillips opened this issue Mar 30, 2023 · 18 comments
Closed

auto-sizing circumgraph image in chrome #38

aphillips opened this issue Mar 30, 2023 · 18 comments
Assignees
Labels
bug Something isn't working

Comments

@aphillips
Copy link
Collaborator

I'm seeing poor spacing when I view the ReSpec ED in Chrome with character images. I couldn't get it to work correctly for the FFFC image and I notice that circumgraph looks like this:

image

Chrome is Version 111.0.5563.112 (Official Build) (64-bit) on Windows 11.

Firefox looks fine. @r12a, any thoughts?

@aphillips aphillips added the bug Something isn't working label Mar 30, 2023
@r12a
Copy link
Contributor

r12a commented Mar 31, 2023

This appears to be because something is adding height and width attributes to the img element in Chrome. The source of the Chrome rendered document says:

<img src="img/circumgraph.svg" alt="କୋ" style="height:1.2rem;" height="150" width="261">

Those attributes are not in the source when displayed by Firefox.

Safari also has the added attributes, but they seem to be calculated based on the result of the style attribute, rather than on the original size of the image (so it looks ok).

The attributes don't appear to be added in my own documents, so i'm inclined to suspect that this is something Respec is doing.

A workaround might be to specify the width as well as the height in the style attribute, or to use height and width attributes to specify the dimensions. You can probably get a figure for the setting from the inspector (Computed).

@r12a
Copy link
Contributor

r12a commented Mar 31, 2023

It may be worth looking into whether there's a way of telling Respec not to do this, or requesting one if there isn't.

@aphillips
Copy link
Collaborator Author

I did try adding the height and width attributes explicitly, but that didn't work!

@r12a
Copy link
Contributor

r12a commented Mar 31, 2023

Strange. Worked for me. I tried adding both in attributes and in styling. Note that attributes can only carry digits representing pixel values.

@aphillips
Copy link
Collaborator Author

aphillips commented Mar 31, 2023

I can reproduce this as an error in Chrome when adding height="150" width="261" (and reversing the order of the attributes). It also does not work when I use just width or just height. When I put the size as height="20rem" width="34.8rem", though, it magically works and the size is set correctly (and the rem sizes or em sizes have an effect on what size the graphic is shown as).

As you note, this is not legal HTML???

@aphillips
Copy link
Collaborator Author

aphillips commented Mar 31, 2023

PS> The rem sizing also works in FF. Editing the height or width in the console has the right visual effect.

image

... edit to double width...:

image

@xfq
Copy link
Member

xfq commented Apr 1, 2023

There seems to be a related bug in ReSpec: https://github.com/w3c/respec/issues/3435

I tried adding attributes to the HTML and that seemed to do the trick, I'll raise a PR so we can see the preview.

@r12a
Copy link
Contributor

r12a commented Apr 5, 2023

This now appears to be fixed. See https://w3c.github.io/i18n-glossary/#dfn-circumgraph in Chrome. Thanks to @sidvishnoi .

Reopen if there's more to say

@r12a r12a closed this as completed Apr 5, 2023
@r12a
Copy link
Contributor

r12a commented Apr 5, 2023

Btw, note that i think it's only fixed for SVG images. If we use the large PNGs in https://github.com/r12a/c to represent characters, we'll probably still need to manually change the attribute dimensions, rather than just fix it by changing a line in the style sheet.

@sidvishnoi
Copy link
Member

@r12a If it causes issues, I think we can add an opt-out mechanism (something like data-respec-ignore or a respecConfig option), or remove this feature altogether.

@r12a
Copy link
Contributor

r12a commented Apr 5, 2023

@sidvishnoi i'd welcome at least an opt out. I think i'd prefer not to add those attributes at all, but i don't know what removing them completely would do to backwards compatibility. For me, such presentation-related metadata shouldn't be in attributes, and should only be in the element tag for truly unique requirements.

I think that, for us, a respecConfig option would be far more useful than having to add a (long) bit of meta-data to every instance. For others, it may be better to have the option to use either(?)

@r12a r12a reopened this Apr 5, 2023
@r12a
Copy link
Contributor

r12a commented Apr 19, 2023

@sidvishnoi any progress on this? I wanted to include an inline png image in a document yesterday, but had to give up. (The original size was much larger than the line, and i wasn't able to reduce the size using CSS.)

@sidvishnoi
Copy link
Member

sidvishnoi commented Apr 19, 2023

Haven't got opportunity yet. Removing that functionality would be a breaking change, and adding another option would be maintenance++ (can't remove it easily in future), so I'm not sure if ReSpec should do either.

As a quick fix, can you add a respecConfig.preProcess function to add width/height attribute so ReSpec ignores those images, and remove those attributes in a postProcess function? ReSpec ignores on these rules:

":not(picture)>img:not([width]):not([height]):not([srcset])"

If not feasible, I might make the breaking change.

@r12a
Copy link
Contributor

r12a commented Apr 20, 2023

If not feasible, I might make the breaking change.

Or provide for a flag to be set in the settings at the top of the page, such as insertHeightWidthAttributes = false ?

@r12a
Copy link
Contributor

r12a commented Jul 27, 2023

Note that Respec no longer has code that automatically adds height and width attributes to img elements.

See https://github.com/w3c/respec/pull/4432

Thanks, @sidvishnoi !

@aphillips
Copy link
Collaborator Author

@xfq: is this fixed, then? Can we close this? Don't forget to delete your branch if it is no longer needed.

@xfq
Copy link
Member

xfq commented Sep 27, 2023

I personally prefer to add height and width to all images to reduce layout shifts, but I don't feel strongly to object and will close this issue.

@xfq xfq closed this as completed Sep 27, 2023
@r12a
Copy link
Contributor

r12a commented Sep 28, 2023

You should still be able to add height and width information to individual images, but respec will no longer force that upon everyone, and will allow flexibility in how it is done which is particularly useful when dealing with sets of images that need to the same dimensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants