[Edbrowse-dev] document.removeAttribute

[Edbrowse-dev] document.removeAttribute

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

Hi all,

As per the [acid3 tests][1] I've implemented what I think is a correct version
of removeAttribute.  My plan is to browse with this for a bit before pushing it
(probably tomorrow), but I've already managed to find a new problem with those
tests.

It seems that, now the js isn't dying due to an undefined function, the page is
using some kind of ajax manipulations.  It means that the page never finishes
loading and the page ends up blank.  If someone can explain the relevant
commands to send a git patch I'll send my version of removeAttribute for review
since it's possible I've made an error which is causing the issue.
Alternatively I can push to my fork of the edbrowse repo and raise a pull
request (though I'm not sure how to raise one with edbrowse).

Cheers,
Adam.
[1]: http://acid3.acidtests.org

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Edbrowse-dev] document.removeAttribute

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> Alternatively I can push to my fork of the edbrowse repo and raise a pull
> request (though I'm not sure how to raise one with edbrowse).

There's a great tool for sending pull requests from the command line.
It's called hub, and I think it's packaged by most distros.
I use this all the time, and it's a breeze.
Just git push to a branch and hub pull-request (with the correct args).

Pull requests are ok, but a patch is probably better, since everyone
will see it.

Also one thing I'm thinking about doing is tightening up our github
integration.  I think I could get github to send mail to this list every
time a pull request or issue is opened, or whenever someone pushes to
the repo.  Would that be appreciated, or would it be too spammy for
everyone?
I already get mail on new issues / pull requests, and I believe that Karl does
as well.

-- Chris

Re: [Edbrowse-dev] document.removeAttribute

[Kevin Carhart]

Adam said,
> page is using some kind of ajax manipulations.  It means that the page

Thank you for implementing removeAttribute-- I'm sure it's needed 
connnnstantly.

Which page is this.. are you doing NASA or Raw Story, etc?
I'll find out soon enough.

Chris said,
> the repo.  Would that be appreciated, or would it be too spammy for

It would be appreciated, I would like to keep up with these.

Kevin

Re: [Edbrowse-dev] document.removeAttribute

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Thu, Jan 19, 2017 at 01:34:08PM -0800, Kevin Carhart wrote:
> 
> Adam said,
> >page is using some kind of ajax manipulations.  It means that the page
> 
> Thank you for implementing removeAttribute-- I'm sure it's needed
> connnnstantly.
> 
> Which page is this.. are you doing NASA or Raw Story, etc?
> I'll find out soon enough.

It's the acid3 tests at:
http://acid3.acidtests.org

Prior to my implementation of removeAttribute it was displaying a bunch of
things with lots of "failed" lines.
Now it's blank and edbrowse gradually slows down to the point of dying
eventually (verified both on NetBSD and Linux).  I've pushed the changes to my
dev fork at:
https://github.com/arthompson/edbrowse.git

I'll double-check my work and try and implement
some kind of testing before pushing it (it's possible I've done something
unfortunate which is causing the issue).

> Chris said,
> >the repo.  Would that be appreciated, or would it be too spammy for
> 
> It would be appreciated, I would like to keep up with these.

Same here, I doubt it'd be too much traffic.  If it is then we could have a
separate edbrowse-repo mailing list in the future.

Cheers,
Adam.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Edbrowse-dev] document.removeAttribute

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Tue, Jan 24, 2017 at 08:43:51AM +0000, Adam Thompson wrote:
> On Thu, Jan 19, 2017 at 01:34:08PM -0800, Kevin Carhart wrote:
> I'll double-check my work and try and implement
> some kind of testing before pushing it (it's possible I've done something
> unfortunate which is causing the issue).

I've pushed to the main edbrowse repo, tested manually but haven't yet had time
to put anything in jsrt.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[Edbrowse-dev] document.removeAttribute

[Karl Dahlke]

I'm a bit confused.
It all works if you remove an attribute that is there,
but if you remove an attribute that isn't there ...

startwindow.js line 233 
c.attributes = new Object;

Not an array but an object.
So if c doesn't have foo then you try to get attributes.length
like it's an array but it's not, so I think that would fail and stop js right there.

Same problem for hasAttribute etc.

Look at decorate.c line 825.
There I make attributes an array.
I think maybe startwindow.js should read

c.attributes = new Array;

That would be consistent.
If so then it's my bug, sorry.
What say you.

Karl Dahlke

Re: [Edbrowse-dev] document.removeAttribute

[Kevin Carhart]

The inconsistency in attributes was one of mine, sorry to miss this.

Re: [Edbrowse-dev] document.removeAttribute

[Kevin Carhart]

On Tue, 24 Jan 2017, Karl Dahlke wrote:

> Honestly I don't know if it's suppose to be an array or an object. Not sure of the spec.
> If an array, does the order of attributes matter?
>

I don't think the order matters.  I think the indices are strings and so 
you would reference them like a hash table of name-value pairs that aren't 
in order.
I found this in the MDN pages,

The Element.attributes property returns a live collection of all attribute 
nodes registered to the specified node. It is a NamedNodeMap, not an 
Array, so it has no Array methods and the Attr nodes' indexes may differ 
among browsers. To be more specific, attributes is a key/value pair of 
strings that represents any information regarding that attribute.

And for NamedNodeMap it says,
The NamedNodeMap interface represents a collection of Attr objects. 
Objects inside a NamedNodeMap are not in any particular order, unlike 
NodeList, although they may be accessed by an index as in an array.



https://developer.mozilla.org/en-US/docs/Web/API/Element/attributes
https://developer.mozilla.org/en-US/docs/Web/API/NamedNodeMap

[Edbrowse-dev] document.removeAttribute

[Karl Dahlke]

Kevin, the last line you quoted from the spec...

> although they may be accessed by an index as in an array.

Ok, if so, then the only practical way to implement that is as an array,
which I did in domLink() in decorate.c,
but we don't do that when creating nodes dynamically from js.
So c. attributes should be new Array, not new Object, but there's more.
In setAttribute() line 635 is either unnecessary or not enough.
We need to preserve the array essence.
Don't we need this?

this.attributes.push(name.toLowerCase());

And in removeAttribute it's fine to delete foo from attributes,
where we had set attributes.foo = bar, but we must also splice foo out of position i in the array.

I'm guessing that if c is the node then there are really three actions to foo = bar.

c.foo = bar
c.attributes.foo = bar
c.attributes.push("foo")

I'm not sure of this, I only know what we have is inconsistent,
and will definitely cause trouble at some point.
If you can confirm that my understanding of this is correct I can clean it up,
or perhaps Kevin or Adam can take it on.

Karl Dahlke

[Edbrowse-dev] document.removeAttribute

[Karl Dahlke]

I think most people just push and hope for the best, or run diff and send the patch around for review.
I'm guessing the page came out blank before, and still comes out blank, with subsequent js still not running,
so that what you did didn't "break" anything, but actually moves us forward.
If so then you should just push it, or use diff and show us a patch if you're feeling cautious.

Karl Dahlke