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

[BUG] changing the viewbox size doesn't seem to expand the code to fit. #222

Closed
fr3nch13 opened this issue Oct 24, 2023 · 17 comments
Closed

Comments

@fr3nch13
Copy link

fr3nch13 commented Oct 24, 2023

Tested on 4.3, 5.0-beta, and main.

Describe the bug or unexpected behaviour

setting the svgViewBoxSize to an int, doesn't seem to fill the viewBox with the QrCode.
Without setting the svgViewBoxSize, the output makes it <svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 41 41" preserveAspectRatio="xMidYMid">.
Setting it, and the only thing that changes is the viewBox="0 0 500 500"

Unfortunately I don't know enough about SVG to know if this is the intended output change, or if there should be more.

Hopefully I'm just overlooking some setting that I should be using.

Steps to reproduce the behavior

Current code, simplified for testing.

    $this->options = new QROptions([
            'svgViewBoxSize' => $this->getConfig('svgViewBoxSize', 500),
            'eccLevel' => $this->getConfig('eccLevel', EccLevel::H),
            'outputType' => $this->getConfig('outputType', QROutputInterface::MARKUP_SVG),
            'svgUseFillAttributes' => $this->getConfig('svgUseFillAttributes', true),
            'drawLightModules' => $this->getConfig('drawLightModules', false),
    ]);
$this->data = "https://localhost";
$optionsLight = clone $this->options;
//$optionsLight->setColor($color);
$qrLight = new ChillerlanQRCode($optionsLight);
$qrLight->render($this->data, $qrImagePathLight);

Expected behavior

That if I set the viewBox size, that the QR code, (and svg logo inside) would expand to fill the view box.

Screenshots
SVG Gist

1-dark

Environment (please complete the following information):

  • PHP version/OS: 8.2
  • Library version: 4.3, 5.0-beta, dev-main

Additional context

@fr3nch13 fr3nch13 added the bug label Oct 24, 2023
@fr3nch13 fr3nch13 changed the title [BUG] changing the voewbox size doesn't seem to expand the code to fit. [BUG] changing the viewbox size doesn't seem to expand the code to fit. Oct 24, 2023
@codemasher
Copy link
Member

Hi. This is not a bug. The expectation that changing the viewbox attribute also changes the fill size of the content is a misconception. The SVG documentation says:

The viewBox attribute defines the position and dimension, in user space, of an SVG viewport.

It's useful to change this attribute if you want to add additional content, e.g. text under the QR symbol. The setting $svgViewBoxSize is deprecated because of this and the preferred method is to override the method QRMarkupSVG::header().

/**
* returns the <svg> header with the given options parsed
*
* @see https://developer.mozilla.org/en-US/docs/Web/SVG/Element/svg
*/
protected function header():string{
$header = sprintf(
'<svg xmlns="http://www.w3.org/2000/svg" class="qr-svg %1$s" viewBox="%2$s" preserveAspectRatio="%3$s">%4$s',
$this->options->cssClass,
$this->getViewBox(),
$this->options->svgPreserveAspectRatio,
$this->eol
);
if($this->options->svgAddXmlHeader){
$header = sprintf('<?xml version="1.0" encoding="UTF-8"?>%s%s', $this->eol, $header);
}
return $header;
}

In order to change the size of the SVG to a fixed, which is what i assume you expect, you need to either add width and height attributes to the svg root element or change its size via CSS.

@codemasher
Copy link
Member

In fact, i should remove this option entirely before releasing v5. I added it a long time ago when i knew nothing about SVG and fell for the same misconception. It is misleading and most likely not widely used anyway.

@fr3nch13
Copy link
Author

fr3nch13 commented Oct 24, 2023

Well, while you were writing this out, I actually did go a similar path, only I overwrote the getViewBox() method so I wasn't overwriting other header info, and possibly no longer be compatible with your original header.

    protected function getViewBox(): string
    {
        $size = parent::getViewBox();
        [$width, $height] = $this->getOutputDimensions();
        $size = sprintf('0 0 %s %s', $width, $height);

        return sprintf('%1$s" width="%2$s" height="%3$s',
            $size,
            ($this->options->svgViewBoxSize ?? $width),
            ($this->options->svgViewBoxSize ?? $height)
        );
    }

Would, instead of svgViewBoxSize, could scale be used here like it is in the other Types like png? Something like:

    /**
     * returns the <svg> header with the given options parsed
     *
     * @see https://developer.mozilla.org/en-US/docs/Web/SVG/Element/svg
     */
    protected function header():string{

        $width = null;
        $height = null;
        if ($this->options->scale) {
            $width = sprintf(' width="%1$s"', $this->options->scale);
            $height = sprintf(' height="%1$s"', $this->options->scale);
        }
        $header = sprintf(
            '<svg xmlns="http://www.w3.org/2000/svg" class="qr-svg %1$s" viewBox="%2$s"%3$s%4$s preserveAspectRatio="%5$s">%6$s',
            $this->options->cssClass,
            $this->getViewBox(),
            $width,
            $height,
            $this->options->svgPreserveAspectRatio,
            $this->eol
        );

        if($this->options->svgAddXmlHeader){
            $header = sprintf('<?xml version="1.0" encoding="UTF-8"?>%s%s', $this->eol, $header);
        }

        return $header;
    }

@codemasher
Copy link
Member

I was experimenting in the past but there's no "best" way to set the size for the SVG, so i decided to leave this to the user entirely. the $scale option in combination with the width and height attributes isn't the best as it would fix the size which is often not preferred (ImagMagick requires them for properly sized output, though). SVG output is almost exclusively used in a web browser where CSS is available, and if it isn't, the size can be changed via CSS in the <defs> element or the aforementioned attributes.

@fr3nch13
Copy link
Author

Sigh, yeah. The whole point of the "S" in SVG. The issue I'm coming across, specific to me. I'm trying to import the QR codes, and the viewport is 0 0 41 41. which gimp makes a 41x41 pixel import. I can scale it up when importing, but it then becomes really fuzzy, like trying to upscale a jpg/png/etc. I was just trying to solve this from an import perspective.

@codemasher
Copy link
Member

That sounds like the same issue i had when converting via ImageMagick - try the output from that example and see what happens.

@codemasher codemasher removed the bug label Oct 24, 2023
@fr3nch13
Copy link
Author

Ok, I just pulled/composer update and I'm running on dev-main.

I copy-pasta'd your svg example here:
https://github.com/chillerlan/php-qrcode/blob/main/examples/svg.php

and getting a viewport of 53 and it's trying to import as a 53x53 image:

<svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 53 53" preserveAspectRatio="xMidYMid">

@fr3nch13
Copy link
Author

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

@codemasher
Copy link
Member

That is correct. I meant try the output from the imagick conversion example with the changed <svg> element.

@codemasher
Copy link
Member

I'm going to close this issue here. Feel free to open a discussion for further questions.

@codemasher
Copy link
Member

codemasher commented Oct 24, 2023

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

Like i said, that is not the main focus or use case for SVG output and the reason why i decided not to add these attributes.

@fr3nch13
Copy link
Author

Oh, derp.

I got: <svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 41 41" preserveAspectRatio="xMidYMid" width="205" height="205">

@fr3nch13
Copy link
Author

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

Like i said, that is not the main focus or use case for SVG and the reason why i decided not to add these attributes.

Gotcha, well at least you made it extendable, and you did a great job on the documentation, I personally appreciate that part! :-p

@codemasher
Copy link
Member

Glad to hear because that's exactly why i'm doing this.

@fr3nch13
Copy link
Author

Well I don't mind being your userland guinea pig.

I'm using it over here, in case you're curious: https://qr.fr3nch.com

@fr3nch13
Copy link
Author

I chose yours, because it's much more configurable, and it runs locally, verse google's graph api.

I also wasn't going to pay bitly and other QR code hosters $35/month when I can make my own site.

@codemasher
Copy link
Member

Ah yes the QR code scammers. Still a wild concept to me so generate a QR code and charge for it on a monthly basis - for tracking statistics you can get for free elsewhere. If i had the time and capacity i'd run such a website for free haha. Oh wait, i did (cheaply)

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

No branches or pull requests

2 participants