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

Jump to bottom

Add urlAttribute helper that uses baseURI (used for href, src etc.) #154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add urlAttribute helper that uses baseURI (used for href, src etc.) #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d54a26e
Add urlAttribute helper, use baseURI on HTMLAnchorElement.href, HTMLI…
danburzo Aug 17, 2022
95bcb02
Fixes
danburzo Aug 17, 2022
fa810f5
Ignore <base> without href for baseURI computation
danburzo Aug 23, 2022
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
6 changes: 3 additions & 3 deletions cjs/html/anchor-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';
const {registerHTMLClass} = require('../shared/register-html-class.js');
const {stringAttribute} = require('../shared/attributes.js');
const {stringAttribute, urlAttribute} = require('../shared/attributes.js');

const {HTMLElement} = require('./element.js');

Expand All @@ -15,8 +15,8 @@ class HTMLAnchorElement extends HTMLElement {
}

/* c8 ignore start */ // copy paste from img.src, already covered
get href() { return encodeURI(stringAttribute.get(this, 'href')); }
set href(value) { stringAttribute.set(this, 'href', decodeURI(value)); }
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }

get download() { return encodeURI(stringAttribute.get(this, 'download')); }
set download(value) { stringAttribute.set(this, 'download', decodeURI(value)); }
Expand Down
6 changes: 6 additions & 0 deletions cjs/html/area-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const {HTMLElement} = require('./element.js');
const {urlAttribute} = require('../shared/attributes.js');

/**
* @implements globalThis.HTMLAreaElement
Expand All @@ -8,5 +9,10 @@ class HTMLAreaElement extends HTMLElement {
constructor(ownerDocument, localName = 'area') {
super(ownerDocument, localName);
}

/* c8 ignore start */
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }
/* c8 ignore stop */
}
exports.HTMLAreaElement = HTMLAreaElement
6 changes: 3 additions & 3 deletions cjs/html/image-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';
const {registerHTMLClass} = require('../shared/register-html-class.js');
const {numericAttribute, stringAttribute} = require('../shared/attributes.js');
const {numericAttribute, stringAttribute, urlAttribute} = require('../shared/attributes.js');

const {HTMLElement} = require('./element.js');

Expand All @@ -21,8 +21,8 @@ class HTMLImageElement extends HTMLElement {
get sizes() { return stringAttribute.get(this, 'sizes'); }
set sizes(value) { stringAttribute.set(this, 'sizes', value); }

get src() { return stringAttribute.get(this, 'src'); }
set src(value) { stringAttribute.set(this, 'src', value); }
get src() { return urlAttribute.get(this, 'src'); }
set src(value) { urlAttribute.set(this, 'src', value); }

get srcset() { return stringAttribute.get(this, 'srcset'); }
set srcset(value) { stringAttribute.set(this, 'srcset', value); }
Expand Down
4 changes: 2 additions & 2 deletions cjs/interface/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class Node extends EventTarget {
const ownerDocument = this.nodeType === DOCUMENT_NODE ?
this : this.ownerDocument;
if (ownerDocument) {
const base = ownerDocument.querySelector('base');
const base = ownerDocument.querySelector('base[href]');
if (base)
return base.getAttribute('href');

Expand All @@ -76,7 +76,7 @@ class Node extends EventTarget {
return location.href;
}

return null;
return "about:blank";
}

/* c8 ignore start */
Expand Down
18 changes: 18 additions & 0 deletions cjs/shared/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,24 @@ const stringAttribute = {
};
exports.stringAttribute = stringAttribute;

const urlAttribute = {
get(element, name) {
const attr = element.getAttribute(name);
if (attr) {
try {
return new URL(attr, element.baseURI).href;
} catch (err) {
return attr;
}
}
return '';
},
set(element, name, value) {
element.setAttribute(name, value);
}
};
exports.urlAttribute = urlAttribute;

/* oddly enough, this apparently is not a thing
export const nullableAttribute = {
get(element, name) {
Expand Down
6 changes: 3 additions & 3 deletions esm/html/anchor-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {registerHTMLClass} from '../shared/register-html-class.js';
import {stringAttribute} from '../shared/attributes.js';
import {stringAttribute, urlAttribute} from '../shared/attributes.js';

import {HTMLElement} from './element.js';

Expand All @@ -14,8 +14,8 @@ class HTMLAnchorElement extends HTMLElement {
}

/* c8 ignore start */ // copy paste from img.src, already covered
get href() { return encodeURI(stringAttribute.get(this, 'href')); }
Copy link
Author

Choose a reason for hiding this comment

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

The getter/setter for href used to encode/decode the URI, and this change breaks some tests. Should this technique be adopted inside urlAttribute?

Browser behavior is below:

let a = document.createElement('a');
a.href = 'https://google.com/?q=asd&lol=<2>';
a.href
// "https://google.com/?q=asd&lol=%3C2%3E"
a.getAttribute('href')
// "https://google.com/?q=asd&lol=<2>"

Should we also store the raw value in the attribute, and encode it on the getter?

Copy link
Owner

Choose a reason for hiding this comment

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

if tests break it means we'll break working code out there ... this change shouldn't require a major though, should it?

Copy link
Owner

Choose a reason for hiding this comment

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

P.S. yes, we might want to encodeURI on the URL returned href, if URL returned href is not encoded already

Copy link
Owner

@WebReflection WebReflection Aug 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

... which it does ...

new URL('/?q=asd&lol=<2>', 'https://google.com/').href
// "https://google.com/?q=asd&lol=%3C2%3E"

so we want this behavior, I would drop the encoding but I am not sure we should drop the decoding ... the encoding is provided by the URL on current attribute which is the raw value, the decoding in assignment looks like still necessary to preserve the raw value ... this should not break anything, just remove the encodeURI in the getter.

Copy link
Author

Choose a reason for hiding this comment

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

I need to read a bit more to understand how to align with the HTML spec (if possible). The immediate fix for the breaking test is:

return new URL(attr, element.baseURI ?? undefined).href;

(baseURI is null in the absence of an explicit location or <base> element, and that's an invalid base URL.)

But the problem is the getter returns either the encoded or unencoded value depending on the validity of baseURI, and I wonder if that's the case in browsers as well.

Copy link
Author

Choose a reason for hiding this comment

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

The current implementation of baseURI is aware of <base> right now, but if speed is a concern this needs to be cached or otherwise optimized, so that might complicate things:

  get baseURI() {
    const ownerDocument = this.nodeType === DOCUMENT_NODE ?
                            this : this.ownerDocument;
    if (ownerDocument) {
      const base = ownerDocument.querySelector('base');
      if (base)
        return base.getAttribute('href');

      const {location} = ownerDocument.defaultView;
      if (location)
        return location.href;
    }

    return null;
  }

Let me know if you think this feature is worth it in relation with your goals / use-cases for linkedom. This has been an interesting rabbit hole regardless!

Copy link
Owner

Choose a reason for hiding this comment

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

uhm ... I see ... then return null should be return 'about:blank' ... right? I don't think this would break much code out there but it is, technically, a breaking change, due returning non falsy value when no case is met ... bummer!

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I don't see a big problem with return 'about:blank'; although it's technically a breaking change, as is I guess changing the behavior of URL-like element attributes. What I'm more concerned is how to get rid of the inefficient querySelector each time you request href on a link, and still update the value when you mutate the DOM.

Copy link
Owner

Choose a reason for hiding this comment

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

The cached version of this module doesn't care about repeated qS/qSA operation if the DOM didn't mutate in the meanwhile, so if that's a concern, just use the cached version instead?

Copy link
Owner

Choose a reason for hiding this comment

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

That is linkedom/cached import, instead of the regular one https://github.com/WebReflection/linkedom#are-childnodes-and-children-always-computed

set href(value) { stringAttribute.set(this, 'href', decodeURI(value)); }
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }

get download() { return encodeURI(stringAttribute.get(this, 'download')); }
set download(value) { stringAttribute.set(this, 'download', decodeURI(value)); }
Expand Down
6 changes: 6 additions & 0 deletions esm/html/area-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {HTMLElement} from './element.js';
import {urlAttribute} from '../shared/attributes.js';

/**
* @implements globalThis.HTMLAreaElement
Expand All @@ -7,4 +8,9 @@ export class HTMLAreaElement extends HTMLElement {
constructor(ownerDocument, localName = 'area') {
super(ownerDocument, localName);
}

/* c8 ignore start */
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }
/* c8 ignore stop */
}
6 changes: 3 additions & 3 deletions esm/html/image-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {registerHTMLClass} from '../shared/register-html-class.js';
import {numericAttribute, stringAttribute} from '../shared/attributes.js';
import {numericAttribute, stringAttribute, urlAttribute} from '../shared/attributes.js';

import {HTMLElement} from './element.js';

Expand All @@ -20,8 +20,8 @@ class HTMLImageElement extends HTMLElement {
get sizes() { return stringAttribute.get(this, 'sizes'); }
set sizes(value) { stringAttribute.set(this, 'sizes', value); }

get src() { return stringAttribute.get(this, 'src'); }
set src(value) { stringAttribute.set(this, 'src', value); }
get src() { return urlAttribute.get(this, 'src'); }
set src(value) { urlAttribute.set(this, 'src', value); }

get srcset() { return stringAttribute.get(this, 'srcset'); }
set srcset(value) { stringAttribute.set(this, 'srcset', value); }
Expand Down
4 changes: 2 additions & 2 deletions esm/interface/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export class Node extends EventTarget {
const ownerDocument = this.nodeType === DOCUMENT_NODE ?
this : this.ownerDocument;
if (ownerDocument) {
const base = ownerDocument.querySelector('base');
Copy link
Author

Choose a reason for hiding this comment

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

<base> elements without a href attribute should be skipped here. The algorithm is a bit more complicated (allowing for relative href), but maybe that can be addressed more thoroughly in a future PR.

const base = ownerDocument.querySelector('base[href]');
if (base)
return base.getAttribute('href');

Expand All @@ -75,7 +75,7 @@ export class Node extends EventTarget {
return location.href;
}

return null;
return "about:blank";
}

/* c8 ignore start */
Expand Down
17 changes: 17 additions & 0 deletions esm/shared/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,23 @@ export const stringAttribute = {
}
};

export const urlAttribute = {
get(element, name) {
const attr = element.getAttribute(name);
if (attr) {
try {
return new URL(attr, element.baseURI).href;
WebReflection marked this conversation as resolved.
Show resolved Hide resolved
} catch (err) {
return attr;
}
}
return '';
},
set(element, name, value) {
element.setAttribute(name, value);
}
};

/* oddly enough, this apparently is not a thing
export const nullableAttribute = {
get(element, name) {
Expand Down
2 changes: 1 addition & 1 deletion test/html/document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ assert(withLocation.document.baseURI, location.href);
const withBase = parseHTML('<html><base href="http://base"></html>', {location});
assert(withBase.document.documentElement.baseURI, 'http://base');

assert((new DocumentFragment).baseURI, null);
assert((new DocumentFragment).baseURI, 'about:blank');

const {document: svg} = (new DOMParser).parseFromString('<svg class="foo-1"/>', 'text/html').defaultView.window;
assert(svg.querySelector('[class*="foo-"]'), svg.firstElementChild);
13 changes: 13 additions & 0 deletions test/html/image-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,16 @@ assert(img.toString(), '<img width="99" src="example.org">', '<img>.width');
assert(img.width, 99);
assert(img.height, 0);
assert(img.lastChild, null, 'lastChild');

const { document: withBase } = parseHTML('<html><img></html>', {
location: {
href: 'https://example.com'
}
});

const { firstElementChild: imgWithBase } = withBase.documentElement;

assert(imgWithBase.src, '', 'Issue #153 - <img>.src with <base>');

imgWithBase.src = 'avatar.png';
assert(imgWithBase.src, 'https://example.com/avatar.png', 'Issue #153 - <img>.src with <base>');
4 changes: 2 additions & 2 deletions types/esm/html/anchor-element.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
* @implements globalThis.HTMLAnchorElement
*/
export class HTMLAnchorElement extends HTMLElement implements globalThis.HTMLAnchorElement {
set href(arg: string);
get href(): string;
set href(arg: any);
get href(): any;
set download(arg: string);
get download(): string;
set target(arg: any);
Expand Down
2 changes: 2 additions & 0 deletions types/esm/html/area-element.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@
* @implements globalThis.HTMLAreaElement
*/
export class HTMLAreaElement extends HTMLElement implements globalThis.HTMLAreaElement {
set href(arg: any);
get href(): any;
}
import { HTMLElement } from "./element.js";
6 changes: 6 additions & 0 deletions types/esm/shared/attributes.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,9 @@ export namespace stringAttribute {
function set(element: any, name: any, value: any): void;
function set(element: any, name: any, value: any): void;
}
export namespace urlAttribute {
function get(element: any, name: any): any;
function get(element: any, name: any): any;
function set(element: any, name: any, value: any): void;
function set(element: any, name: any, value: any): void;
}
34 changes: 28 additions & 6 deletions worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4168,6 +4168,23 @@ const stringAttribute = {
}
};

const urlAttribute = {
get(element, name) {
const attr = element.getAttribute(name);
if (attr) {
try {
return new URL(attr, element.baseURI).href;
} catch (err) {
return attr;
}
}
return '';
},
set(element, name, value) {
element.setAttribute(name, value);
}
};

/* oddly enough, this apparently is not a thing
export const nullableAttribute = {
get(element, name) {
Expand Down Expand Up @@ -4324,7 +4341,7 @@ class Node$1 extends DOMEventTarget {
const ownerDocument = this.nodeType === DOCUMENT_NODE ?
this : this.ownerDocument;
if (ownerDocument) {
const base = ownerDocument.querySelector('base');
const base = ownerDocument.querySelector('base[href]');
if (base)
return base.getAttribute('href');

Expand All @@ -4333,7 +4350,7 @@ class Node$1 extends DOMEventTarget {
return location.href;
}

return null;
return "about:blank";
}

/* c8 ignore start */
Expand Down Expand Up @@ -10948,8 +10965,8 @@ class HTMLImageElement extends HTMLElement {
get sizes() { return stringAttribute.get(this, 'sizes'); }
set sizes(value) { stringAttribute.set(this, 'sizes', value); }

get src() { return stringAttribute.get(this, 'src'); }
set src(value) { stringAttribute.set(this, 'src', value); }
get src() { return urlAttribute.get(this, 'src'); }
set src(value) { urlAttribute.set(this, 'src', value); }

get srcset() { return stringAttribute.get(this, 'srcset'); }
set srcset(value) { stringAttribute.set(this, 'srcset', value); }
Expand Down Expand Up @@ -11010,6 +11027,11 @@ class HTMLAreaElement extends HTMLElement {
constructor(ownerDocument, localName = 'area') {
super(ownerDocument, localName);
}

/* c8 ignore start */
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }
/* c8 ignore stop */
}

/**
Expand Down Expand Up @@ -11041,8 +11063,8 @@ class HTMLAnchorElement extends HTMLElement {
}

/* c8 ignore start */ // copy paste from img.src, already covered
get href() { return encodeURI(stringAttribute.get(this, 'href')); }
set href(value) { stringAttribute.set(this, 'href', decodeURI(value)); }
get href() { return urlAttribute.get(this, 'href'); }
set href(value) { urlAttribute.set(this, 'href', value); }

get download() { return encodeURI(stringAttribute.get(this, 'download')); }
set download(value) { stringAttribute.set(this, 'download', decodeURI(value)); }
Expand Down