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

Switched order of attribute initialization and DOM addition. #24006

Merged
merged 1 commit into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.server.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.webassembly.js

Large diffs are not rendered by default.

47 changes: 35 additions & 12 deletions src/Components/Web.JS/src/Rendering/BrowserRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,8 @@ export class BrowserRenderer {
document.createElementNS('http://www.w3.org/2000/svg', tagName) :
document.createElement(tagName);
const newElement = toLogicalElement(newDomElementRaw);
insertLogicalChild(newDomElementRaw, parent, childIndex);

let inserted = false;

// Apply attributes
const descendantsEndIndexExcl = frameIndex + frameReader.subtreeLength(frame);
Expand All @@ -247,24 +248,49 @@ export class BrowserRenderer {
if (frameReader.frameType(descendantFrame) === FrameType.attribute) {
this.applyAttribute(batch, componentId, newDomElementRaw, descendantFrame);
} else {
insertLogicalChild(newDomElementRaw, parent, childIndex);
inserted = true;
// As soon as we see a non-attribute child, all the subsequent child frames are
// not attributes, so bail out and insert the remnants recursively
this.insertFrameRange(batch, componentId, newElement, 0, frames, descendantIndex, descendantsEndIndexExcl);
break;
}
}

// We handle setting 'value' on a <select> in two different ways:
// [1] When inserting a corresponding <option>, in case you're dynamically adding options
// this element did not have any children, so it's not inserted yet.
if (!inserted) {
insertLogicalChild(newDomElementRaw, parent, childIndex);
}

// We handle setting 'value' on a <select> in three different ways:
// [1] When inserting a corresponding <option>, in case you're dynamically adding options.
// This is the case below.
// [2] After we finish inserting the <select>, in case the descendant options are being
// added as an opaque markup block rather than individually
// Right here we implement [2]
if (newDomElementRaw instanceof HTMLSelectElement && selectValuePropname in newDomElementRaw) {
// added as an opaque markup block rather than individually. This is the other case below.
// [3] In case the the value of the select and the option value is changed in the same batch.
// We just receive an attribute frame and have to set the select value afterwards.

if (newDomElementRaw instanceof HTMLOptionElement)
{
// Situation 1
this.trySetSelectValueFromOptionElement(newDomElementRaw);
} else if (newDomElementRaw instanceof HTMLSelectElement && selectValuePropname in newDomElementRaw) {
// Situation 2
const selectValue: string | null = newDomElementRaw[selectValuePropname];
setSelectElementValue(newDomElementRaw, selectValue);
}
}

private trySetSelectValueFromOptionElement(optionElement: HTMLOptionElement) {
const selectElem = this.findClosestAncestorSelectElement(optionElement);
if (selectElem && (selectValuePropname in selectElem) && selectElem[selectValuePropname] === optionElement.value) {
setSelectElementValue(selectElem, optionElement.value);
delete selectElem[selectValuePropname];
return true;
}
return false;
}

private insertComponent(batch: RenderBatch, parent: LogicalElement, childIndex: number, frame: RenderTreeFrame) {
const containerElement = createAndInsertLogicalContainer(parent, childIndex);

Expand Down Expand Up @@ -385,13 +411,10 @@ export class BrowserRenderer {
} else {
element.removeAttribute('value');
}

// See above for why we have this special handling for <select>/<option>
// Note that this is only one of the two cases where we set the value on a <select>
const selectElem = this.findClosestAncestorSelectElement(element);
if (selectElem && (selectValuePropname in selectElem) && selectElem[selectValuePropname] === value) {
this.tryApplyValueProperty(batch, selectElem, attributeFrame);
delete selectElem[selectValuePropname];
}
// Situation 3
this.trySetSelectValueFromOptionElement(<HTMLOptionElement>element);
return true;
}
default:
Expand Down
4 changes: 4 additions & 0 deletions src/Components/test/E2ETest/Tests/BindTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@ public void CanBindSelect()
Browser.Equal("Fourth", () => boundValue.Text);
Assert.Equal("Fourth choice", target.SelectedOption.Text);

// verify that changing an option value and selected value at the same time works.
Browser.FindElement(By.Id("change-variable-value")).Click();
Browser.Equal("Sixth", () => boundValue.Text);

// Verify we can select options whose value is empty
// https://github.com/dotnet/aspnetcore/issues/17735
target.SelectByText("Empty value");
Expand Down
11 changes: 11 additions & 0 deletions src/Components/test/E2ETest/Tests/EventTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,17 @@ public void EventDuringBatchRendering_CannotTriggerJSInterop()
Browser.Contains(expectedMessage, () => errorLog.Text);
}

[Fact]
public void RenderAttributesBeforeConnectedCallBack()
{
Browser.MountTestComponent<RenderAttributesBeforeConnectedCallback>();
var element = Browser.FindElement(By.TagName("custom-web-component-data-from-attribute"));

var expectedContent = "success";

Browser.Contains(expectedContent, () => element.Text);
}

void SendKeysSequentially(IWebElement target, string text)
{
// Calling it for each character works around some chars being skipped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@
<optgroup label="Some choices">
<!-- Show it also works with optgroup -->
<option value=@string.Empty>Empty value</option>
<option value="@variableValue">Variable value</option>
<option value=@SelectableValue.First>First choice</option>
<option value=@SelectableValue.Second>Second choice</option>
<option value=@SelectableValue.Third>Third choice</option>
Expand All @@ -253,6 +254,7 @@
</select>
<span id="select-box-value">@selectValue</span>
<button id="select-box-add-option" @onclick="AddAndSelectNewSelectOption">Add and select new item</button>
<button id="change-variable-value" @onclick="ChangeVariableValueToSixth">Change variable value to Sixth.</button>
</p>

<h2>Select (markup block)</h2>
Expand Down Expand Up @@ -383,13 +385,21 @@
DateTime? timeStepTextboxNullableDateTimeValue = null;

bool includeFourthOption = false;
enum SelectableValue { First, Second, Third, Fourth }
enum SelectableValue { First, Second, Third, Fourth, Fifth, Sixth }
SelectableValue? selectValue = SelectableValue.Second;
SelectableValue? selectMarkupValue = SelectableValue.Second;

SelectableValue variableValue = SelectableValue.Fifth;

void AddAndSelectNewSelectOption()
{
includeFourthOption = true;
selectValue = SelectableValue.Fourth;
}

void ChangeVariableValueToSixth()
{
selectValue = SelectableValue.Sixth;
variableValue = SelectableValue.Sixth;
}
}
1 change: 1 addition & 0 deletions src/Components/test/testassets/BasicTestApp/Index.razor
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
<option value="BasicTestApp.RedTextComponent">Red text</option>
<option value="BasicTestApp.ReliabilityComponent">Server reliability component</option>
<option value="BasicTestApp.RenderFragmentToggler">Render fragment renderer</option>
<option value="BasicTestApp.RenderAttributesBeforeConnectedCallback">Render attributes before ConnectedCallback</option>
<option value="BasicTestApp.ReorderingFocusComponent">Reordering focus retention</option>
<option value="BasicTestApp.RouterTest.NavigationManagerComponent">NavigationManager Test</option>
<option value="BasicTestApp.RouterTest.TestRouter">Router</option>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<h1>Render Attributes Before connectedCallback</h1>

<p>
When the connectedcallback event fires, it's nice to have the attributes of the HTML element set already,
as data is usually passed to the component using custom attributes.
</p>


<custom-web-component-data-from-attribute myattribute="@attributeString"></custom-web-component-data-from-attribute>


@code {
string attributeString = "success";
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

<!-- Used for specific test cases -->
<script src="js/jsinteroptests.js"></script>
<script src="js/renderattributestest.js"></script>
<script src="js/webComponentPerformingJsInterop.js"></script>

<script>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// This web component is used from the RenderAttributesBeforeConnectedCallback test case

window.customElements.define('custom-web-component-data-from-attribute', class extends HTMLElement {
connectedCallback() {
let myattribute = this.getAttribute('myattribute') || 'failed';

this.innerHTML = myattribute;
}
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

<!-- Used for specific test cases -->
<script src="js/jsinteroptests.js"></script>
<script src="js/renderattributestest.js"></script>
<script src="js/webComponentPerformingJsInterop.js"></script>

<div id="blazor-error-ui">
Expand Down