Where here I'm adding the ability to hide the stuff which isn't actually sent if the URL is changed too much. (Currently report broken site doesn't do so).

I followed the lead of ipprotection-customelements.js for the component, and felt it might be worth having this intermediate file in case we add more components in the future.

I consolidated and cleaned up a bit of CSS here, as we only have one error label now which is not in a web component (for the comment/description textarea).

Not needed now that we fit the panels to their height/content.

Looks like you don't really need to pass this and can just do the checks on

I don't really understand this, can you elaborate further what is going on here? Don't you just want to only descend into the shadow root if .

This is the default anyway so can be skipped.

Should this be returning ?

Add a JSdoc comment describing the paramaters please

I'd prefer this to be a specific property rather than relying on the ordering on the property.

At this point is empty so is there any point in calling this?

Here you remove the pref observers when there are no browser windows left. But when a new browser window opens you don't re-register the observers.

Is this intentionally left in?

Should this be within the if block above?

All of these string IDs contain a spurious .

Yes, that's what this is doing (the walker always descends into shadow roots, so I have to skip them myself first before asking it for the next/prev node, if the current one isn't skipped).

I suppose not. Will remove.

Great catch. I'll fix that.

Nope, will remove.

Yes.

I'd feel more comfortable about the logic in here if we had a straightforward way to test this directly. One option would be to write a chrome mochitest for it.

Do we need to update to here?

Ok I see. Can you just do something like this though?

if (shadowRoot && !this.isSkippedNode(currentNode)) {
 this.#walker.showAnonymousContent = false;
}

let nextNode = this.#walker.nextNode();
this.#walker.showAnonymousContent = true;

while (nextNode && this.isSkippedNode(nextNode)) {
 nextNode = this.#walker.nextNode();
}

return nextNode;

already updates the so I think this simplifies to this?

It's not ideal to have custom behaviour for the report broken site specific element here, but I'm also not seeing why this is needed, can you explain what it solves?

If we don't do this, then the cursor in the input doesn't move if we press left/right arrow keys. I tried using a more generic fix like this at first, but it broke other unrelated tests in tree, like browser/components/ipprotection/tests/browser/browser_ipprotection_keyboard_navigation.js

Does setting on the element fix that?

Does setting on the element fix that?

Unforuntately no, the entire component gets focus, rather than the input inside of it.

I was able to get the same result with this, which makes sense, it just ensures we always get the actual element with focus. Does that work for you? If not your approach is fine.

Either way there is an odd issue with focus ordering. If I am editing the url and press tab then focus moves to the panel close button above, pressing tab a second time moves back to the url input and then again moves to the next element.

Yes, that seems to work fine. Will change.

would make sense to whitelist this, perhaps just file a bug on that? (I'd do it but I'm on PTO, so I'm not really here)

could this use inset-inline-start?

this seems too generic in combination with :has which is often considered harmful for performance. can you set an extra class on this element instead?

can this use the child selector?

nit: combine into one :not with comma

Done. https://bugzilla.mozilla.org/show_bug.cgi?id=2039619

Actually I don't even need this anymore after other changes, so let's drop it entirely.

I'm not super confident that I can alter how the viewcontainers set height with JS without causing other bugs.. are you okay with us doing this in a follow-up, or is the perf risk too high to land this as-is?

I feel like using Lit state properties might be a better fit here? https://lit.dev/docs/components/properties/#internal-reactive-state

static properties = {
 url: { type: String, state: true },
 favicon: { type: String, state: true },
}

might simplify some of the manual work happening with the property creation/setters.

also it's generally best practice to avoid class fields in Lit elements due to some potential footguns: https://lit.dev/docs/components/properties/#avoiding-issues-with-class-fields

You could simplify this by just adding event listeners directly in the template e.g. https://lit.dev/docs/components/events/#adding-event-listeners-in-the-element-template

This doesn't seem to have any effect when I click on it?

I had tried this initially, but it wasn't working properly to carry over early state before the component was rendered, so I came up with this magic goop instead. However, I tried again and did find out how to do it properly, so I'll make this change in the final patch.

Ah, I unintentionally broke it a bit while converting to a lit component. Will be fixed in the final patch.

FYI, this landed with a broken migration, which means it wasn't tested (the version approved by @bolsson was completely different).

Turns out the migration is only copying 2 strings because all the other directives are missing a .

To be clear, there is no point in updating the migration now. For the future, if you completely change a patch after it's been reviewed, please ask for another review.

Will do, sorry :(

If there anything I can do at this point to help (write a follow-up migration etc) please let me know.

would be good to have s follow-up on this.

Agreed, I just had not created a bug for it yet. I just filed https://bugzilla.mozilla.org/show_bug.cgi?id=2040437.